-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor the cli functions to use urfave/cli instead of cobra. #218
Conversation
Codecov Report
@@ Coverage Diff @@
## main #218 +/- ##
==========================================
- Coverage 40.29% 40.11% -0.19%
==========================================
Files 46 46
Lines 2169 2179 +10
==========================================
Hits 874 874
- Misses 1236 1246 +10
Partials 59 59
Continue to review full report at Codecov.
|
f3541f1
to
72e14fa
Compare
bc5ee7f
to
49d5d24
Compare
} | ||
|
||
logger := log.GetLogger(context.Context) | ||
logger.Infof( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably move the version output to the root command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually how does this work? Is there the equivalent of the persistent pre-run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no persistent pre-run
or persistent flag
. App.Before
does what pre-run
does. And we have access to parent flags through the context.
Now I think moving this to root command (app) is a good idea. But it will be shown only when calling that command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
App.before
on the App root will run on any subcommand right?
// An action to execute before any subcommands are run, but after the context is ready
// If a non-nil error is returned, no subcommands are run
Before BeforeFunc
https://github.com/urfave/cli/blob/v1.22.5/app.go#L28
Or is that what you're saying? Maybe I'm not understanding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry I forgot the context of this comment. But yeah App.before
always run. A subCommand can also declare command.Before
.
|
||
if err := cmdflags.AddHiddenFlagsToCommand(cmd, cfg); err != nil { | ||
return nil, fmt.Errorf("adding hidden flags to run command: %w", err) | ||
if cfg.ParentIface == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does urfave/cli
handle required fields and raising an error if its missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes there is required: true
, but the way it behave is that you have to set the flag manually, I think the check happens before the command.before
where we set the flag with the config file, causing it to fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to set a config file and flags at the same time? which takes precedence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The precedence for flag value sources is as follows (highest to lowest):
- Command line flag value from user
- Environment variable (if specified)
- Configuration file (if specified)
- Default defined on the flag
dd6917b
to
2225d51
Compare
A few variable have been renamed to match, e.g. cfg.CtrKernelSnapshotter to match defaults.ContainerdKernelSnapshotter Signed-off-by: Soule BA <soule@weave.works>
2225d51
to
1c80a95
Compare
This PR is stale because it has been open 30 days with no activity. |
I need to properly review this |
if err := rootCmd.Execute(); err != nil { | ||
log.Fatalln(err) | ||
if err := app.Run(os.Args); err != nil { | ||
fmt.Fprintf(os.Stderr, "flintlockd: %s\n", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be log.Error
?
This PR is stale because it has been open 30 days with no activity. |
This PR is stale because it has been open 30 days with no activity. |
Is this still in progress ? |
Lets close this for now |
Signed-off-by: Soule BA soule@weave.works
kind/refactor
What this PR does / why we need it:
Refactor the cli functions to use urfave/cli instead of cobra.
Viper is replaced by urfave/cli atlsrc.
A few variable have been renamed to match their default conterparts, e.g.
cfg.CtrKernelSnapshotter to match defaults.ContainerdKernelSnapshotter
this fixes #81
Special notes for your reviewer:
Checklist: