-
Notifications
You must be signed in to change notification settings - Fork 11
Version fix #96
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
Version fix #96
Conversation
Signed-off-by: Joseph <jvaikath@redhat.com>
Signed-off-by: Joseph <jvaikath@redhat.com>
shubham-pampattiwar
left a comment
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.
Nice work on this! The regex approach is clean and the tests look solid.
Question about the Short/Long descriptions
I noticed the help output still shows "Velero" in the command descriptions:
$ ./kubectl-oadp --help
Available Commands:
bug Report a Velero bug
client Velero client related commands
create Create velero resources
version Print the velero version and associated imageWas this intentional or should these also say "OADP"?
I can see it both ways:
Keeping "Velero":
- Makes sense if we're thinking of
oadpas a CLI wrapper around Velero - Like how
kubectlis the CLI but you're still managing "Kubernetes" - "Report a Velero bug" = bug in the Velero project
- "Create velero resources" = Velero CRDs
Changing to "OADP":
- Better consistency - users only see "OADP" everywhere
- Simplifies the mental model
- They installed oadp-cli, not velero-cli
Right now your PR fixes:
- ✅ Examples in help text
- ✅ Runtime messages like
Run \velero backup describe`` - ❓ Command descriptions (Short/Long fields)
What's your take - should we update those descriptions too, or leave them referencing Velero?
Signed-off-by: Joseph <jvaikath@redhat.com>
|
@shubham-pampattiwar |
|
yeah thats what I was inclined towards too, Thank you ! @Joeavaikath |
Signed-off-by: Joseph <jvaikath@redhat.com>
|
/lgtm |
kaovilai
left a comment
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.
lgtm
Why the changes were made
How to test the changes made
Build and run any command to see the changes