feat: [NPM] Add config file and feature toggles #979
Conversation
npm/cmd/main.go
Outdated
|
|
||
| func init() { | ||
| cobra.OnInitialize(initConfig) | ||
| rootCmd.Flags().StringVar(&cfgFile, "config", "", "config file (default is $HOME/.azure-npm-debug-cli.yaml)") |
There was a problem hiding this comment.
help string is misleading, maybe:
| rootCmd.Flags().StringVar(&cfgFile, "config", "", "config file (default is $HOME/.azure-npm-debug-cli.yaml)") | |
| rootCmd.Flags().StringVar(&cfgFile, "config", "", fmt.Sprintf("config file (default is %s)", npmconfig.GetConfigPath())) |
npm/cmd/main.go
Outdated
| config := &npmconfig.Config{} | ||
| err := viper.Unmarshal(config) | ||
| if err != nil { | ||
| fmt.Printf("unable to decode into config struct, %v", err) |
There was a problem hiding this comment.
since this is a RunE, probably better to wrap and return
npm/cmd/main.go
Outdated
| fmt.Printf("unable to decode into config struct, %v", err) | ||
| } | ||
|
|
||
| Start(*config) |
There was a problem hiding this comment.
similarly, this func should block or return an error, and we should return Start(...)
npm/cmd/main.go
Outdated
| var cfgFile string | ||
|
|
||
| // rootCmd represents the base command when called without any subcommands | ||
| var rootCmd = &cobra.Command{ |
There was a problem hiding this comment.
very subjective, but i prefer to put the root command in a root.go, and keep main.go very lean, with only a main() and the min cobra imports
| @@ -1,4 +1,4 @@ | |||
| package cmd | |||
| package main | |||
There was a problem hiding this comment.
the package is cmd, isn't it? and we can drop the "cmd" from this filename.
npm/util/errors/errors.go
Outdated
| ) | ||
|
|
||
| var ( | ||
| SrcNotSpecified = errors.New("source not specified") |
There was a problem hiding this comment.
should these be in the package where they get used instead of a util?
|
|
||
| // Start starts shared informers and waits for the shared informer cache to sync. | ||
| func (npMgr *NetworkPolicyManager) Start(stopCh <-chan struct{}) error { | ||
| func (npMgr *NetworkPolicyManager) Start(config npmconfig.Config, stopCh <-chan struct{}) error { |
There was a problem hiding this comment.
should we include this argument if we're not using the config here yet?
There was a problem hiding this comment.
this is a preliminary change, it will be required when subsequent pr's with resourcehandlers behind toggles are added
|
|
||
| restserver := restserver.NewNpmRestServer(restserver.DefaultHTTPListeningAddress) | ||
| go restserver.NPMRestServerListenAndServe(npMgr) | ||
| go restserver.NPMRestServerListenAndServe(config, npMgr) |
There was a problem hiding this comment.
delete "restserver." since you made this a function instead of a method
There was a problem hiding this comment.
restserver here is indicative of the package import where NPMRestServerListenAndServe lives, previously in this section it was overloaded as both package import and var declaration
matmerr
force-pushed
the
npmconfigfile
branch
2 times, most recently
from
79c61c6 to
cbee786
Compare
matmerr
force-pushed
the
npmconfigfile
branch
3 times, most recently
from
a5cbe2a to
9386b1a
Compare
4 participants
Reason for Change:
Issue Fixed:
Requirements:
Notes: