-
Notifications
You must be signed in to change notification settings - Fork 372
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
Add custom usage template for cobra command so that usage will display 'kubectl krew' instead of just 'krew' #547
Conversation
Welcome @brianpursley! |
/assign @corneliusweig |
By the way, I waffled on whether to just bring in the entire template from cobra and maintain it in krew or to use string.Replace(). I ended up with string.Replace() because it was less code and seemed a little more targeted. Let me know if you have a preference. Here is their template:
|
Honestly, my only concern is that this value isn't a const in cobra, and I'm not sure if they treat the template as "API" that they shouldn't be breaking. But this works for me. Worth adding a note "why" we're doing this in the code (with links to relevant issues/PRs). |
Added comment with link to issue |
/retest |
@brianpursley: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Can I get ok-to-test? |
/ok-to-test That test failure looks like a flake. We should look into it if reproes. Feel free to open an issue with the stage’s output. |
Can we add a note to the |
Not sure why the tests's failing, do you mind quickly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like your solution to modify the template string. Cobra moves so slowly that we are safe here 😅
One more idea: if it breaks, we won't know unless somebody happens to notice it. So could you modify one of our integration tests to look out for kubectl krew
in the command output?
Argh not sure if that's gonna be a clean test. :| :| |
Isn't it good enough to see if the substring |
I will see if I can add some kind of test like you said. I think it is a good idea. |
@ahmetb @corneliusweig I made the proposed changes. Can you take another look. (Also, it looks like the test that was failing before has passed, so maybe the rebase fixed it) |
nope. There's already a |
…y 'kubectl krew' instead of just 'krew' See issue kubernetes-sigs#521 for more details
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brianpursley, corneliusweig The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
# Files generated by JetBrains IDEs, e.g. IntelliJ IDEA | ||
.idea/ | ||
*.iml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brianpursley I just noticed this.
Ideally we don't wanna have editor-specific stuff here.
You should add these to your ~/.gitignore_global file and set git global config core.excludesfile=~/.gitignore_global
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that. I will make a PR to clean it up :)
Fixes #521
Custom usage template lets us add
kubectl
in front of the usagesExample output of default usage message with this PR:
Example output of a command help with this PR in place: