Skip to content

Migrate urfave/cli to v3 + add manpage subcommand #1969

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Migrate urfave/cli to v3 + add manpage subcommand #1969

wants to merge 7 commits into from

Conversation

@folliehiyuki
Copy link

@folliehiyuki folliehiyuki commented Oct 12, 2025

The main motivation of this PR is better support for shell completion. We won't need to maintain custom completion scripts, while having additional support for fish shell and powershell.

References:

@folliehiyuki
feat: migrate urfave/cli to v3
f90d5ad 
Signed-off-by: Hoang Nguyen <[email protected]>
@folliehiyuki
chore: NewExitError() -> Exit()
aa8674c 
to match the underlying cli.Exit() function it calls.

Signed-off-by: Hoang Nguyen <[email protected]>
@folliehiyuki
fix: remove unused parameter path in loadStoresConfig() function
c2ce957 
Signed-off-by: Hoang Nguyen <[email protected]>
@folliehiyuki
Copy link
Author

folliehiyuki commented Oct 12, 2025

I noticed that urfave/cli v3 removed a hidden builtin feature to generate CLI documentation and put it into a separated Go package. Even though this is a bit out of scope for this PR, I added another commit to include a subcommand to generate man page for sops.

@folliehiyuki folliehiyuki changed the title Migrate urfave/cli to v3 Migrate urfave/cli to v3 + add manpage subcommand Oct 12, 2025
felixfontein
felixfontein requested changes Oct 12, 2025
View reviewed changes
Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

The last time I did investigated github.com/urfave/cli/v3 it was processing arguments sufficiently different that this would be a breaking change for the CLI interface of SOPS, since it parses some things quite differently, and thus not acceptable. Is there now some compatibility mode with v1? Or is this PR a breaking change?

@folliehiyuki
feat: add manpage subcommand
0f1cc3e 
Signed-off-by: Hoang Nguyen <[email protected]>
@folliehiyuki
Copy link
Author

folliehiyuki commented Oct 12, 2025 •
edited
Loading

The last time I did investigated github.com/urfave/cli/v3 it was processing arguments sufficiently different

Can you explain the behavior? Here is some differences I spotted:

  • flags must be specified before arguments
  • there is no longer a concept of global flags. Flags can be passed through to subcommands by default

Apart from those, I don't see anything strange (yet). I only use a small subset of sops' functionalities, so I haven't tested every subcommand. Whether the above changes are breaking or not is up to you. Personally it doesn't affect my daily usage.

@folliehiyuki
Copy link
Author

folliehiyuki commented Oct 12, 2025 •
edited
Loading

There are also differences in how --help flag and help subcommand are used (help subcommand can be invoked by another subcommand by default), as well as how the help text is displayed for root command and subcommands. If you desire the old help behavior, there are workarounds.

@folliehiyuki
fix: properly set alternate flag names using Aliases
eea878e 
Signed-off-by: Hoang Nguyen <[email protected]>
@folliehiyuki
chore: mark all root command flags as Local
b1fce04 
With this change, we don't need the Go template hack anymore. The help
text for subcommands displays correctly without showing global options.

Signed-off-by: Hoang Nguyen <[email protected]>
@folliehiyuki
fix: remove duplicated --verbose flag declaration for subcommands
c395aa5 
It's already defined as a root command flag.

Signed-off-by: Hoang Nguyen <[email protected]>
@folliehiyuki
Copy link
Author

folliehiyuki commented Oct 13, 2025

Even with Local = true set for all global flags, they still don't behave the same as the current sops command (global flags can still be specified anywhere in the command line).

I opened urfave/cli#2208 as I don't know whether this is an intended behavior.

folliehiyuki
folliehiyuki commented Oct 13, 2025
View reviewed changes
cmd/sops/main.go
Sources: cli.EnvVars("SOPS_DECRYPTION_ORDER"),
},
}, keyserviceFlags...),
// Repeat keyserviceFlags, with Local value set to true
Copy link
Author

@folliehiyuki folliehiyuki Oct 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I was tempted to inject Local = true into all global flags after they are defined, but the approach seems to be messy, and we probably will need to wrap the slice elements with a custom interface. Every global flag is defined plainly here, even with a level of duplication, for simplicity.

Ref: urfave/cli#2073

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felixfontein felixfontein felixfontein requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@folliehiyuki @felixfontein