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

Added a small utility method to display a warnings. #16441

Merged
merged 5 commits into from Jul 27, 2022

Conversation

maxcoulombe
Copy link
Contributor

@maxcoulombe maxcoulombe commented Jul 25, 2022

Will print warning if flag is passed after positional arguments e.g. vault <command> -a b -c
In this example -c will be interpreted as an argument because go-flag stops parsing after the first non-flag arg.

This behaviour may cause confusion or commands to run with unexpected results.
The decision to display a warning instead of crashing was to avoid potentially breaking existing scripts and integrations.

flag_warning

Small clean-up:

  • Uniformized receiver name as f

Will print warning if flag is passed after arguments e.g.
vault <command> -a b -c
In this example -c will be interpreted as an argument which may be misleading
@hashicorp-cla
Copy link

hashicorp-cla commented Jul 25, 2022

CLA assistant check
All committers have signed the CLA.

@maxcoulombe maxcoulombe marked this pull request as draft July 25, 2022 19:07
@maxcoulombe maxcoulombe marked this pull request as ready for review July 26, 2022 19:28
@maxcoulombe maxcoulombe requested a review from a team July 26, 2022 19:28
command/base.go Outdated
Comment on lines 592 to 595
printArgsWarningIfAny(&out, f.Args())
if out.String() != "" {
f.ui.Warn(out.String())
}
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, you can have printArgsWarningIfAny return the warning as a string, and output the string if it's non-empty.

Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

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

Looks good! Just some small suggestions

changelog/16441.txt Outdated Show resolved Hide resolved
command/base_helpers.go Outdated Show resolved Hide resolved
command/base_helpers.go Outdated Show resolved Hide resolved
* returning string value instead of using output buffer
* rephrased warning message for clarity
Copy link
Member

@austingebauer austingebauer left a comment

Choose a reason for hiding this comment

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

LGTM with the recent commits 👍

@maxcoulombe maxcoulombe merged commit 2166d6e into main Jul 27, 2022
@calvn calvn modified the milestones: 1.11.2, 1.12.0-rc1 Jul 27, 2022
@calvn calvn deleted the vault-2165-args-warning branch July 27, 2022 20:20
@maxcoulombe maxcoulombe mentioned this pull request Aug 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

5 participants