Modularize config factory #8505
Conversation
N-o-Z
added
the
exclude-changelog
N-o-Z
force-pushed
the
task/modularize-config-8497
branch
5 times, most recently
from
ecd2356 to
4b3f9e4
Compare
E2E Test Results - DynamoDB Local - Local Block Adapter |
E2E Test Results - Quickstart |
N-o-Z
force-pushed
the
task/modularize-config-8497
branch
from
4b3f9e4 to
22ece2d
Compare
N-o-Z
force-pushed
the
task/modularize-config-8497
branch
from
22ece2d to
f8071bc
Compare
cmd/lakefs/cmd/root.go
Outdated
| @@ -52,6 +53,8 @@ func init() { | |||
| rootCmd.PersistentFlags().Bool(config.QuickstartConfiguration, false, "Use lakeFS quickstart configuration") | |||
| } | |||
|
|
|||
| // TODO (niro): All this validation logic should be in the OSS config package | |||
There was a problem hiding this comment.
When should this happen?
Is there an Issue for it?
There was a problem hiding this comment.
It's not something that is blocking us from continuing work. This is something we should do to cleanup the code a little.
There was a problem hiding this comment.
Can you please change the comment to not refer to "OSS"? 🙏
pkg/config/config.go
Outdated
| } `mapstructure:"gs"` | ||
| } | ||
|
|
||
| type Interface interface { |
There was a problem hiding this comment.
This is quite confusing.
Can you rename Config to BaseConfig, and Interface to Config?
There was a problem hiding this comment.
The renaming of Config will create a lot of unnecessary changes in the code I'd like to avoid.
If you think we should renaming it still let me know (since you'll be the one needing to deal with the headache of reviewing it 😆 )
There was a problem hiding this comment.
I assumed this is the reason.
I'd be happy to review such change -
It will only mess this single PR, and will make our code more readable.
modules/config/factory/build.go
Outdated
| return newConfig(cfgType) | ||
| } | ||
|
|
||
| func newConfig(cfgType string) (*config.Config, error) { |
There was a problem hiding this comment.
Kind of nit, but -
Why this func is needed?
Why this isn't the code of BuildConfig?
modules/config/factory/build.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| // OSS specific validation |
There was a problem hiding this comment.
I don't think we should have such "OSS" references in the code.
Maybe something like "mandatory validation", or "basic validation" -
Otherwise this comment will only confuse maintainers.
…fig-8497 # Conflicts: # pkg/config/config_test.go
Closes #8497
Change Description
Extract config creation into a factory module