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.

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

disable printing flags warning message for the ssh command #20502

Merged
merged 5 commits into from
May 8, 2023
Merged

disable printing flags warning message for the ssh command #20502

merged 5 commits into from
May 8, 2023

Conversation

hghaf099
Copy link
Contributor

@hghaf099 hghaf099 commented May 4, 2023 •
edited
Loading

Addresses #19785

Echoing what @maxb has mentioned in the ticket:

The vault ssh command has the unusual purpose of being a wrapper that invokes the real ssh command (after obtaining credentials). 
Moreover, the ssh command can take another command on its command line, to run on the remote machine. 
Because of this, there are multiple ways in which it can be a normal use-case for there to be option-like arguments later in the command line, 
which are deliberately intended either for ssh, or for a command being run via ssh on the remote machine.

This is quite unlike most other commands, where option-like things occurring after positional arguments are often a mistake.

@hghaf099 hghaf099 requested review from maxcoulombe and a team May 4, 2023 02:54
miagilepner
miagilepner reviewed May 4, 2023
View reviewed changes
Copy link
Contributor

@miagilepner miagilepner left a comment

Choose a reason for hiding this comment

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

implementation looks good. would it be possible to add a test for this?

@hghaf099 hghaf099 requested a review from miagilepner May 4, 2023 15:35
@hghaf099
Copy link
Contributor Author

hghaf099 commented May 4, 2023

implementation looks good. would it be possible to add a test for this?

Good call. Test added!

@hghaf099 hghaf099 enabled auto-merge (squash) May 5, 2023 22:40
@hghaf099 hghaf099 merged commit c93f4aa into main May 8, 2023
This was referenced May 8, 2023
Closed
Closed
Merged
hghaf099 added a commit that referenced this pull request May 8, 2023
@hghaf099
disable printing flags warning message for the ssh command (#20502)
5f8d3a5 
* disable printing flags warning message for the ssh command

* adding a test

* CL

* add go doc on the test
@hghaf099 hghaf099 deleted the ssh-command-disable-flag-warning-message branch May 8, 2023 17:20
@hghaf099 hghaf099 mentioned this pull request May 8, 2023
Merged
hghaf099 added a commit that referenced this pull request May 11, 2023
@hghaf099
disable printing flags warning message for the ssh command (#20502) (#…
fd8005b 
…20544)

* disable printing flags warning message for the ssh command

* adding a test

* CL

* add go doc on the test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miagilepner miagilepner miagilepner approved these changes

@maxcoulombe maxcoulombe Awaiting requested review from maxcoulombe

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