feat: Improve config generation #318
Conversation
fmoura
force-pushed
the
feature/imporve-config-code-generation
branch
from
d5da941
to
a78d9e0
Compare
There was a problem hiding this comment.
This is looking great! It'll help me log the node's configs easily now in #319.
Since I had the time, I've taken the liberty to propose a few changes in #323. I suggest checking the commit messages for extra context. I'd also suggest changing the final commit message to something more descriptive, like: feat: aggregate node config in a single struct.
After the PR is merged, I'd argue we would need a follow-up issue to refactor config package into something more cohesive. The cache feature is mostly useless as it is now, for example. But that's a discussion for later 😉
fmoura
force-pushed
the
feature/imporve-config-code-generation
branch
3 times, most recently
from
0b23315
to
77d3e37
Compare
internal/config/generate/main.go
Outdated
|
|
||
| addLine(&doc, "<!-- markdownlint-disable MD012 -->") | ||
| //Validate envs | ||
| for i := range envs { |
There was a problem hiding this comment.
for _, env := range envs {
There was a problem hiding this comment.
as Validate sets the default value, if we use _,env we get a copy of the "Env" and the value is not correctly set. Unless we do .validate() on every loop and use this Env instance to generate that code snippet.
There was a problem hiding this comment.
It seems strange to me that NodeConfig gets initialized from the environment with all its fields set. Are there no optional fields? I think this has to do with my prior comment on the RedisEndpoint.
Before, the program would explode if we required an env that was not set and had no default. But that was ok because we only called Get when the program had to explode if an env was not set and had no default.
But now we always call all gets. Won't that fail in production?
I think some fields should be *type and the constructor for NodeConfig should call
getOptional. Then we can have a (not generated) function Validate() that checks whether we have a valid node config instance.
Make sense?
I agree that we should refactor |
fmoura
force-pushed
the
feature/imporve-config-code-generation
branch
from
77d3e37
to
9d00b7f
Compare
fmoura
force-pushed
the
feature/imporve-config-code-generation
branch
from
9d00b7f
to
311d8fa
Compare
Since this PR will break the main branch*, I think it is best to merge the other config PRs on top of this branch. And then we continue with the proposed changes. (It breaks the main because in reader mode, the auth variables do not need to bet set. Thus, the struct initializer would panic.) |
fmoura
force-pushed
the
feature/imporve-config-code-generation
branch
from
058b500
to
b9a3169
Compare
fmoura
force-pushed
the
feature/imporve-config-code-generation
branch
4 times, most recently
from
189fe65
to
d862fdd
Compare
fmoura
force-pushed
the
feature/imporve-config-code-generation
branch
from
35b2c80
to
c19ca1b
Compare
fmoura
force-pushed
the
feature/imporve-config-code-generation
branch
from
c19ca1b
to
2c11f31
Compare
There was a problem hiding this comment.
After some consideration, I changed my mind about merging this PR and dealing with its implications in a follow-up issue (#325).
As we've noticed multiple times, the old design of the config package doesn't fit our current needs, and adapting it will inevitably create technical debt, something we should avoid.
Even though we've already spent a great amount of time and effort with this, I'm convinced that going back to the drawing board and creating a design from scratch that better accounts for the new requirements is a better approach.
closes #309