Skip to content
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

Upgrade github.com/urfave/cli to v2 #6628

Closed
wants to merge 1 commit into from
Closed

Conversation

kzys
Copy link
Member

@kzys kzys commented Mar 7, 2022

While urfave/cli doesn't have any issues, it is better to keep our dependencies up-to-date.

Signed-off-by: Kazuyoshi Kato katokazu@amazon.com

@k8s-ci-robot
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@kzys kzys force-pushed the cli-upgrade branch 10 times, most recently from cc5b1b6 to a042162 Compare March 7, 2022 21:24
@kzys
Copy link
Member Author

kzys commented Mar 7, 2022

@AkihiroSuda Do we have flags that take a negative value as a string? Seems the issue in go.mod (urfave/cli#1092) was raised for runc.

Name: "run",
Usage: "run a container",
ArgsUsage: "[flags] Image|RootFS ID [COMMAND] [ARG...]",
SkipArgReorder: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an equivalent for v2?

Copy link
Member Author

@kzys kzys Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the flag reordering is removed from v2.

https://github.com/urfave/cli/blob/45952a7d1b0e956db2099493fe1a42837ff5bd0d/docs/CHANGELOG.md#1190---2016-11-19

SkipArgReorder was added to allow users to skip the argument reordering. This is useful if you want to consider all "flags" after an argument as arguments rather than flags (the default behavior of the stdlib flag library). This is backported functionality from the removal of the flag reordering in the unreleased version 2

urfave/cli#398

Given the tradeoffs I think we should remove support for flag reordering.

While urfave/cli doesn't have any issues, it is better to keep our
dependencies up-to-date.

Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
@kzys kzys marked this pull request as ready for review March 7, 2022 22:26
@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 7, 2022

Build succeeded.

  • containerd-build-arm64 : RETRY_LIMIT in 3m 40s (non-voting)
  • containerd-test-arm64 : RETRY_LIMIT in 5m 51s (non-voting)
  • containerd-integration-test-arm64 : RETRY_LIMIT in 24m 40s (non-voting)

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Mar 8, 2022

I feel https://github.com/spf13/cobra is more actively maintained and has better shell completion support (bash, zsh, fish, powershell)
nerdctl already migrated from urfave/cli/v2 to spf13/cobra .

@kzys
Copy link
Member Author

kzys commented Mar 8, 2022

Interesting. I honestly don't have strong opinions regarding cobra vs. urfave/cli. Let me take a look.

@jterry75
Copy link
Contributor

jterry75 commented Mar 8, 2022

@AkihiroSuda - Do we need to go to v2 first though? So should we still take this?

@kzys
Copy link
Member Author

kzys commented Mar 8, 2022

I think, we don't have to do v1 -> v2 migration before urfave/cli -> cobra migration.

@kzys kzys closed this Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants