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

🐛Add deprecation message to clusterctl commands #1439

Merged
merged 1 commit into from
Sep 25, 2019

Conversation

wfernandes
Copy link
Contributor

Signed-off-by: Warren Fernandes wfernandes@pivotal.io

What this PR does / why we need it:
Add deprecation message to each clusterctl command

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1423

NOTES
A side effect of setting the Deprecated property on cobra.Command is that in the --help of the commands, it does not list the available sub-commands. The sub-commands will still run and print out the deprecation message at the top. Scripts/automation will not break.

@k8s-ci-robot
Copy link
Contributor

Welcome @wfernandes!

It looks like this is your first PR to kubernetes-sigs/cluster-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 24, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @wfernandes. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 24, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Sep 24, 2019
Error: unknown flag: --invalid-flag
Usage:
clusterctl create [command]
Copy link
Member

Choose a reason for hiding this comment

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

I don't necessarily mind this diff, but this could cause issues for users of clusterctl in that the usage does not actually show the deprecated subcommands now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had the same feeling about degrading the user experience of clusterctl --help commands.

The behavior of kubectl run only shows the deprecation message when executing the command and not in any help or usage output. We can follow the same behavior, however I feel it would be useful to display the deprecation message as part of the help output. I have another solution where we use a custom help template. I'll re-submit the PR and we can see how that feels.

Signed-off-by: Warren Fernandes <wfernandes@pivotal.io>
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 25, 2019
@@ -44,14 +46,16 @@ var deleteClusterCmd = &cobra.Command{
Long: `Delete a kubernetes cluster with one command`,
Run: func(cmd *cobra.Command, args []string) {
if do.KubeconfigPath == "" {
exitWithHelp(cmd, "Please provide kubeconfig file for cluster to delete.")
exitWithHelp(cmd, "Please provide kubeconfig file for cluster to delete.\n")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added these newlines so that the output would be easier to read.

$ ./clusterctl delete cluster
Please provide kubeconfig file for cluster to delete.

NOTICE: clusterctl has been deprecated in v1alpha2 and will be removed in a future version.
Delete a kubernetes cluster with one command
...

@wfernandes
Copy link
Contributor Author

I updated the PR such that the deprecation message would show when a command is ran and also during the help.
This update also shows all available sub-commands and flags in the help output that way the user experience is not degraded and users can still use the help output of each command.

@detiber
Copy link
Member

detiber commented Sep 25, 2019

/ok-to-test
/approve
/assign @vincepri

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 25, 2019
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/ok-to-test
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: detiber, vincepri, wfernandes

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@vincepri
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 25, 2019
@k8s-ci-robot k8s-ci-robot merged commit b0e9d37 into kubernetes-sigs:master Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add deprecation message to each clusterctl command
4 participants