cmd: Support a Configuration File #2831
Conversation
mqasimsarfraz
force-pushed
the
qasim/configuration
branch
from
f09904f
to
f0ed3d9
Compare
Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>
Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>
Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>
mqasimsarfraz
force-pushed
the
qasim/configuration
branch
from
f0ed3d9
to
ff615ff
Compare
| // we don't care about the error here, as the config file is optional | ||
| v.ReadInConfig() |
There was a problem hiding this comment.
Hum...
I would still print a warning, I would like to avoid a bad situation where not printing this message could lead to really bad stuff occurring.
| } | ||
|
|
||
| func bindFlags(command *cobra.Command, v *viper.Viper) error { | ||
| command.Flags().VisitAll(func(f *pflag.Flag) { |
There was a problem hiding this comment.
Does this return an error? If so, catch it and return it.
Otherwise, this function never returns an error, and the above test will never be true.
| // 1. Flags | ||
| // 2. Config file [/etc/ig/config.yaml, $HOME/.ig/config.yaml, ./config.yaml] | ||
| // 3. Defaults |
There was a problem hiding this comment.
Do we also need to deal with environment variables here?
| { | ||
| name: "flags precedence over defaults", | ||
| args: []string{"--flag", "fvalue", "--flag-with-default", "fvalue"}, | ||
| wantFlag: "fvalue", | ||
| wantFlagDefault: "fvalue", | ||
| }, | ||
| { | ||
| name: "flag precedence over config", | ||
| args: []string{"--flag", "fvalue", "--flag-with-default", "fvalue"}, | ||
| config: testConfig, | ||
| wantFlag: "fvalue", | ||
| wantFlagDefault: "fvalue", | ||
| }, |
There was a problem hiding this comment.
Currently, these tests are the same.
Indeed, you create the config in all cases.
So, in order, you will get:
- The default values.
- The config ones.
- The flag ones.
For the first test, you will need a test without any config or an empty config.
To ease everything, you can add a member config to the test case and handle it in the execution of the test only if present.
| if cmd.Flags().Lookup(key) == nil { | ||
| log.Debugf("Ignoring unused config key: %s", key) |
There was a problem hiding this comment.
If I understand correctly, this is equivalent to giving an unknown flag on the CLI?
If that so, we should rather explode, as it is the case with unknown flag.
Thanks for the feedback!
Just fyi, I am planning to make some adjustments (like introducing a config package) so perhaps wait a bit till I push those changes, thanks! |
Support a Configuration File for
kubectl-gadget,gadgetctlandig. It will also allow to specifykubectl gadget deployflags as config file (should it be done) and defines following priority order:[/etc/ig/config.yaml, $HOME/.ig/config.yaml, ./config.yaml]Also, it is print a debug log for unused keys to avoid surprises.
Related: #2493
Testing Done
TODO: