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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

step cli flags with aliases sometimes report a different alias in error messages #821

Open
weaversam8 opened this issue Jan 3, 2023 · 2 comments

Comments

@weaversam8
Copy link

Hello!

  • Vote on this issue by adding a 馃憤 reaction
  • If you want to implement this feature, comment to let us know (we'll work with you on design, scheduling, etc.)

Issue details

When using the step ssh proxycommand command in step CLI, I tried to configure a --provisioner flag, but accidentally provided the wrong provisioner name. When I did so, the error message I received was:

invalid value '<wrong provisioner name>' for flag '--issuer'

This initially confused me, as I didn't realize that --issuer and --provisioner are aliases for the same flag:

cli/flags/flags.go

Lines 156 to 160 in 160bd07

// Provisioner is a cli.Flag used to pass the CA provisioner to use.
Provisioner = cli.StringFlag{
Name: "provisioner,issuer",
Usage: "The provisioner <name> to use.",
}

Error messages should provide the name of the flag that was used, not the name of an alias that was not used.

Why is this needed?

Inaccurate error messages are misleading and can cause a developer to believe they've made a mistake in a location other than the actual source of the problem.

@weaversam8 weaversam8 added enhancement needs triage Waiting for discussion / prioritization by team labels Jan 3, 2023
@dopey
Copy link
Contributor

dopey commented Jan 6, 2023

Hey @weaversam8 馃憢. Thanks for opening the issue!

First off, you're totally right, and this has also confused me multiple times.

Second, for anyone reading this, we would definitely accept a PR to resolve this, as long as the PR didn't replace the underlying flag library that is being used (this would be a much broader change).

@dopey dopey added good first issue and removed needs triage Waiting for discussion / prioritization by team labels Jan 6, 2023
@dopey dopey added this to the Backlog milestone Jan 6, 2023
@ap-kulkarni
Copy link

ap-kulkarni commented Oct 5, 2023

I did some analysis for this. From what I have understood, I think the information about which flag alias was passed on the command line is lost after the parsing of command line is complete. The flag data structure, in which the data about the flag passed on the command line lies, does not have any information about what was passed on command line. Also, neither of the structs cli.Context, cli.App & cli.Command save the actual command line, from which we could have cross-checked/extracted the alias of the flag.
One way to mitigate this could be to amend the help/error text and include all the aliases instead of only one. e.g. The error mentioned in issue description could look like
invalid value '<wrong provisioner name>' for flag '--issuer'/'--provisioner'
However, this would be a broader a change (in the sense that all such help/error messages involving aliases will need to be amended). But it won't be a change in logic, so there won't be a much of a testing overhead.

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

No branches or pull requests

3 participants