Refactor the cli functions to use urfave/cli instead of cobra. #218

souleb · 2021-11-05T11:11:29Z

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:

squashed commits into logical changes
includes documentation
adds unit tests
adds or updates e2e tests

codecov-commenter · 2021-11-05T11:14:51Z

Codecov Report

Merging #218 (f099f13) into main (aa5916e) will decrease coverage by 0.18%.
The diff coverage is 0.00%.

❗ Current head f099f13 differs from pull request most recent head 1c80a95. Consider uploading reports for the commit 1c80a95 to get more accurate results

@@            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

Impacted Files	Coverage Δ
pkg/log/flags.go	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17de35e...1c80a95. Read the comment docs.

richardcase · 2021-11-16T14:23:23Z

internal/command/gw/gw.go

+		}
+
+		logger := log.GetLogger(context.Context)
+		logger.Infof(


We could probably move the version output to the root command?

Actually how does this work? Is there the equivalent of the persistent pre-run?

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

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

I'm sorry I forgot the context of this comment. But yeah App.before always run. A subCommand can also declare command.Before.

internal/command/root.go

richardcase · 2021-11-16T14:37:09Z

internal/command/run/run.go


-	if err := cmdflags.AddHiddenFlagsToCommand(cmd, cfg); err != nil {
-		return nil, fmt.Errorf("adding hidden flags to run command: %w", err)
+		if cfg.ParentIface == "" {


Does urfave/cli handle required fields and raising an error if its missing?

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

is it possible to set a config file and flags at the same time? which takes precedence?

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

A few variable have been renamed to match, e.g. cfg.CtrKernelSnapshotter to match defaults.ContainerdKernelSnapshotter Signed-off-by: Soule BA <soule@weave.works>

github-actions · 2021-12-18T07:21:31Z

This PR is stale because it has been open 30 days with no activity.

richardcase · 2021-12-20T12:07:01Z

I need to properly review this

jmickey · 2022-01-12T14:15:13Z

cmd/flintlockd/main.go

-	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)


Should this be log.Error?

github-actions · 2022-02-20T07:20:43Z

This PR is stale because it has been open 30 days with no activity.

github-actions · 2023-05-19T07:19:50Z

This PR is stale because it has been open 30 days with no activity.

yitsushi · 2023-05-25T05:24:44Z

Is this still in progress ?

souleb marked this pull request as draft November 5, 2021 11:12

souleb force-pushed the cobra-to-urfavecli branch from f3541f1 to 72e14fa Compare November 5, 2021 16:28

yitsushi added the kind/refactor Only refactoring changes label Nov 8, 2021

souleb force-pushed the cobra-to-urfavecli branch 10 times, most recently from bc5ee7f to 49d5d24 Compare November 12, 2021 11:26

souleb marked this pull request as ready for review November 12, 2021 11:26

richardcase reviewed Nov 16, 2021

View reviewed changes

souleb force-pushed the cobra-to-urfavecli branch 4 times, most recently from dd6917b to 2225d51 Compare November 17, 2021 09:33

Refactor the cli functions to use urfave/cli instead of cobra.

1c80a95

A few variable have been renamed to match, e.g. cfg.CtrKernelSnapshotter to match defaults.ContainerdKernelSnapshotter Signed-off-by: Soule BA <soule@weave.works>

souleb force-pushed the cobra-to-urfavecli branch from 2225d51 to 1c80a95 Compare November 17, 2021 09:38

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 18, 2021

Callisto13 mentioned this pull request Dec 22, 2021

Use flintlock configfile in systemd service #337

Closed

jmickey reviewed Jan 12, 2022

View reviewed changes

richardcase removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 20, 2022

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 20, 2022

Callisto13 removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 21, 2022

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the cli functions to use urfave/cli instead of cobra. #218

Refactor the cli functions to use urfave/cli instead of cobra. #218

souleb commented Nov 5, 2021 •

edited

codecov-commenter commented Nov 5, 2021 •

edited

richardcase Nov 16, 2021

richardcase Nov 16, 2021

souleb Nov 16, 2021

jmickey Jan 12, 2022

souleb Jan 19, 2022

richardcase Nov 16, 2021

souleb Nov 16, 2021

Callisto13 Jan 11, 2022

souleb Jan 19, 2022

github-actions bot commented Dec 18, 2021

richardcase commented Dec 20, 2021

jmickey Jan 12, 2022 •

edited

github-actions bot commented Feb 20, 2022

github-actions bot commented May 19, 2023

yitsushi commented May 25, 2023

Refactor the cli functions to use urfave/cli instead of cobra. #218

Are you sure you want to change the base?

Refactor the cli functions to use urfave/cli instead of cobra. #218

Conversation

souleb commented Nov 5, 2021 • edited

codecov-commenter commented Nov 5, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 18, 2021

richardcase commented Dec 20, 2021

jmickey Jan 12, 2022 • edited

Choose a reason for hiding this comment

github-actions bot commented Feb 20, 2022

github-actions bot commented May 19, 2023

yitsushi commented May 25, 2023

souleb commented Nov 5, 2021 •

edited

codecov-commenter commented Nov 5, 2021 •

edited

jmickey Jan 12, 2022 •

edited