Viper and Cobra integration #51
Conversation
jshencode
requested review from
lucafuji,
shz117 and
voyager-dw
as code owners
cmd/cmd.go
Outdated
| scope.Counter("restart").Inc(1) | ||
| serverRestartTimer := scope.Timer("restart").Start() | ||
|
|
||
| // Create MetaStore. | ||
| metaStorePath := filepath.Join(*rootPath, "metastore") | ||
| metaStorePath := filepath.Join(utils.GetConfig().RootPath, "metastore") |
There was a problem hiding this comment.
config is already passed in
Codecov Report
@@ Coverage Diff @@
## master #51 +/- ##
==========================================
- Coverage 75.76% 75.54% -0.22%
==========================================
Files 85 86 +1
Lines 13189 13253 +64
==========================================
+ Hits 9992 10012 +20
- Misses 2392 2435 +43
- Partials 805 806 +1
Continue to review full report at Codecov.
|
| } | ||
|
|
||
| // ResetDefaults reset default config, logger and metrics settings | ||
| func ResetDefaults() { | ||
| logger = common.NewLoggerFactory().GetDefaultLogger() | ||
| queryLogger = common.NewLoggerFactory().GetDefaultLogger() | ||
| scope := tally.NewTestScope("test", nil) |
There was a problem hiding this comment.
these are default settings used in tests mostly
in production we set using utils.Init()
| v.SetConfigFile(cfgFile) | ||
| } else { | ||
| v.SetConfigName("ares") | ||
| v.AddConfigPath("./config") |
There was a problem hiding this comment.
how about customized config path?
| // AppParams contains application parameters for creating command | ||
| type AppParams struct { | ||
| DefaultCfg map[string]interface{} | ||
| ServerLogger bark.Logger |
There was a problem hiding this comment.
maybe we can enhance this by checking ServerLogger, QueryLogger, Metrics is nil or not and set default accordingly
cmd/cmd.go
Outdated
| DefaultCfg map[string]interface{} | ||
| ServerLogger bark.Logger | ||
| QueryLogger bark.Logger | ||
| Metrics common.Metrics |
There was a problem hiding this comment.
instead of using a big struct, probably we can use optional argument pattern here:
https://github.com/tmrts/go-patterns/blob/master/idiom/functional-options.md
so user can specify what they want and leave defaults for remaining
cmd/cmd.go
Outdated
|
|
||
| loggerFactory := common.NewLoggerFactory() | ||
| if options.ServerLogger == nil { | ||
| options.ServerLogger = loggerFactory.GetDefaultLogger() |
There was a problem hiding this comment.
nits, we can create defaults during struct initialization:
like the example in the link did
utils/config.go
Outdated
| v.BindEnv("env") | ||
| } | ||
|
|
||
| // AddFlagsToCommand adds flags to command |
utils/config.go
Outdated
| "os" | ||
| ) | ||
|
|
||
| // BindenvsToViper binds environment variables to viper |
No description provided.