kubectl: fix deprecation warning bug #46062

alexandercampbell · 2017-05-18T23:50:57Z

What this PR does / why we need it:

Some kubectl commands were deprecated but would fail to print the
correct warning message when a flag was given before the command name.

# Correctly prints the warning that "resize" is deprecated and
# "scale" is now preferred.
kubectl resize [...]

# Should print the same warning but no warning is printed.
kubectl --v=1 resize [...]

This was due to a fragile check on os.Args[1].

This commit implements a new function deprecatedCmd() that is used to
construct new "passthrough" commands which are marked as deprecated and
hidden.

Note that there is an existing "filters" system that may be preferable
to the system created in this commit. I'm not sure why the "filters"
array was not used for all deprecated commands in the first place.

Release note:

NONE

Some kubectl commands were deprecated but would fail to print the correct warning message when a flag was given before the command name. # Correctly prints the warning that "resize" is deprecated and # "scale" is now preferred. kubectl scale [...] # Should print the same warning but no warning is printed. kubectl --v=1 scale [...] This was due to a fragile check on os.Args[1]. This commit implements a new function deprecatedCmd() that is used to construct new "passthrough" commands which are marked as deprecated and hidden. Note that there is an existing "filters" system that may be preferable to the system created in this commit. I'm not sure why the "filters" array was not used for all deprecated commands in the first place.

k8s-ci-robot · 2017-05-18T23:50:59Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

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. I understand the commands that are listed here.

k8s-ci-robot · 2017-05-18T23:51:04Z

Hi @alexandercampbell. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot 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.

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. I understand the commands that are listed here.

alexandercampbell · 2017-05-19T00:01:58Z

@k8s-ci-robot signed CLA

pwittrock · 2017-05-19T00:07:55Z

clever

pwittrock · 2017-05-19T00:08:05Z

@k8s-bot ok to test

pwittrock · 2017-05-19T00:29:54Z

Looks good. Are there existing tests for this? If not, add some to https://github.com/kubernetes/kubernetes/blob/master/hack/make-rules/test-cmd-util.sh

alexandercampbell · 2017-05-19T00:31:24Z

@pwittrock awesome, thanks. I was going to ask where tests for this should be written.

droot · 2017-05-19T00:41:39Z

LGTM

droot · 2017-05-19T00:40:34Z

pkg/kubectl/cmd/cmd.go

@@ -284,6 +284,7 @@ func NewKubectlCommand(f cmdutil.Factory, in io.Reader, out, err io.Writer) *cob
 				NewCmdCreate(f, out, err),
 				NewCmdExposeService(f, out),
 				NewCmdRun(f, in, out, err),
+				deprecatedCmd("run-container", NewCmdRun(f, in, out, err)),


droot · 2017-05-19T16:41:19Z

pkg/kubectl/cmd/cmd.go

+	cmd.Hidden = true
+	return cmd
+}
+


was thinking more about the unit test for this function. We can write a unit test which wraps a NoOp cobra.Command and captures the "deprecated" string in the output. Thoughts ? (happy to discuss in person)

Sounds good to me. Will look at that.

Unit test written. Thanks for the suggestion!

This will clear up some of the confusion around the deprecatedCmd / Deprecated function.

There's no reason to export this function, so I've made it private.

pwittrock · 2017-05-19T18:31:01Z

@alexandercampbell Write a PTAL comment when you are ready to have us take another look

alexandercampbell · 2017-05-19T18:33:31Z

Thanks! I will do that as soon as I figure out what's up with the CI.

droot

Took a quick look at the unit test and have a few comments.

droot · 2017-05-19T19:36:45Z

pkg/kubectl/cmd/cmd_test.go

@@ -25,9 +25,12 @@ import (
 	"net/http"
 	"os"
 	"reflect"
+	stdstrings "strings"


Is there any advantage of aliasing "strings" pkg here ?

The name strings is taken by k8s.io/kubernetes/pkg/util/strings which is also imported in this package.

Got it. makes sense.

droot · 2017-05-19T21:05:10Z

pkg/kubectl/cmd/cmd_test.go

+		t.Errorf("original command has name %q, expected %q",
+			original.Name(), "print")
+	}
+


hmm... I was thinking more along the lines of....invoking the deprecated command and then grabbing the output and check if deprecated warning is being emitted ?

We could also include the output writer as a dependency defaulted to stdout, but overridden the test, and see what bytes that it writes.

I've updated the test to include more coverage-- see 5e1a3fd.

Just looked at it that commit. Looks good to me!

Two new behaviors are tested: 1. The output message that deprecatedAlias gives when it is called must include the word "deprecatated" and the name of the new function that the user should use instead. 2. The correct function must be called by the alias (alias should "fall back" to the functionality of the original.

droot

LGTM.

droot · 2017-05-19T22:08:36Z

pkg/kubectl/cmd/cmd_test.go

@@ -25,9 +25,12 @@ import (
 	"net/http"
 	"os"
 	"reflect"
+	stdstrings "strings"


Got it. makes sense.

droot · 2017-05-19T22:10:37Z

pkg/kubectl/cmd/cmd_test.go

+		t.Errorf("original command has name %q, expected %q",
+			original.Name(), "print")
+	}
+


Just looked at it that commit. Looks good to me!

Additionally, move the test down to ensure definition order matches run order.

k8s-ci-robot · 2017-05-22T21:24:32Z

@alexandercampbell: The following test(s) failed:

Test name	Commit	Details	Rerun command
Jenkins unit/integration	`5e1a3fd`	link	`@k8s-bot unit test this`
Jenkins verification	`5e1a3fd`	link	`@k8s-bot verify test this`
Jenkins kops AWS e2e	`5e1a3fd`	link	`@k8s-bot kops aws e2e test this`
pull-kubernetes-federation-e2e-gce	`b7cc618`	link	`@k8s-bot pull-kubernetes-federation-e2e-gce test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

alexandercampbell · 2017-05-22T22:07:57Z

I tentatively believe that the tests are green.
PTAL, and let me know if you guys want me to rebase out the extraneous commits.

pwittrock · 2017-05-23T00:30:40Z

/lgtm

pwittrock · 2017-05-23T00:30:43Z

/approve

k8s-github-robot · 2017-05-23T00:39:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexandercampbell, pwittrock

Needs approval from an approver in each of these OWNERS Files:

~~docs/OWNERS~~ [pwittrock]
~~hack/OWNERS~~ [pwittrock]
~~pkg/kubectl/OWNERS~~ [pwittrock]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-05-23T03:58:06Z

Automatic merge from submit-queue (batch tested with PRs 46201, 45952, 45427, 46247, 46062)

…precation-errors Automatic merge from submit-queue (batch tested with PRs 46201, 45952, 45427, 46247, 46062) kubectl: fix deprecation warning bug **What this PR does / why we need it**: Some kubectl commands were deprecated but would fail to print the correct warning message when a flag was given before the command name. # Correctly prints the warning that "resize" is deprecated and # "scale" is now preferred. kubectl resize [...] # Should print the same warning but no warning is printed. kubectl --v=1 resize [...] This was due to a fragile check on os.Args[1]. This commit implements a new function deprecatedCmd() that is used to construct new "passthrough" commands which are marked as deprecated and hidden. Note that there is an existing "filters" system that may be preferable to the system created in this commit. I'm not sure why the "filters" array was not used for all deprecated commands in the first place. **Release note**: ```release-note NONE ```

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 18, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 18, 2017

k8s-github-robot assigned fabianofranz and pwittrock May 18, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels May 18, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 19, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 19, 2017

droot approved these changes May 19, 2017

View reviewed changes

Alexander Campbell added 4 commits May 19, 2017 08:55

kubectl: improve docstring on deprecatedCmd

b3fc655

test-cmd-util.sh: add test for deprecated commands

2acc5f1

Merge branch 'master' into correct-deprecation-errors

acfdafb

.generated_docs: update to include "new" commands

699a3e2

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 19, 2017

droot reviewed May 19, 2017

View reviewed changes

Alexander Campbell added 3 commits May 19, 2017 10:40

kubectl: renmae deprecatedCmd -> deprecatedAlias

bf2fc62

This will clear up some of the confusion around the deprecatedCmd / Deprecated function.

kubectl: make "Deprecated" a private function

2f0faed

There's no reason to export this function, so I've made it private.

kubectl: write unit test for deprecatedAlias()

ca899ec

Alexander Campbell added 2 commits May 19, 2017 12:58

Merge branch 'master' into correct-deprecation-errors

213f3c7

Merge branch 'master' into correct-deprecation-errors

4055425

droot reviewed May 19, 2017

View reviewed changes

Alexander Campbell added 2 commits May 19, 2017 14:20

Merge branch 'master' into correct-deprecation-errors

487398e

droot reviewed May 19, 2017

View reviewed changes

Alexander Campbell added 3 commits May 22, 2017 09:55

Merge branch 'master' into correct-deprecation-errors

c9c8d61

test-cmd-util: fix deprecated commands test

46cdb39

Additionally, move the test down to ensure definition order matches run order.

docs: add new generated docs for deprecated commands

b7cc618

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 23, 2017

k8s-github-robot merged commit 644a544 into kubernetes:master May 23, 2017

alexandercampbell deleted the correct-deprecation-errors branch May 23, 2017 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubectl: fix deprecation warning bug #46062

kubectl: fix deprecation warning bug #46062

alexandercampbell commented May 18, 2017 •

edited

k8s-ci-robot commented May 18, 2017

k8s-ci-robot commented May 18, 2017

alexandercampbell commented May 19, 2017

pwittrock commented May 19, 2017

pwittrock commented May 19, 2017

pwittrock commented May 19, 2017

alexandercampbell commented May 19, 2017

droot commented May 19, 2017

droot May 19, 2017

droot May 19, 2017

alexandercampbell May 19, 2017

alexandercampbell May 19, 2017

pwittrock commented May 19, 2017

alexandercampbell commented May 19, 2017

droot left a comment

droot May 19, 2017

alexandercampbell May 19, 2017

droot May 19, 2017

droot May 19, 2017

pwittrock May 19, 2017

alexandercampbell May 19, 2017

droot May 19, 2017

droot left a comment

droot May 19, 2017

droot May 19, 2017

k8s-ci-robot commented May 22, 2017 •

edited

alexandercampbell commented May 22, 2017

pwittrock commented May 23, 2017

pwittrock commented May 23, 2017

k8s-github-robot commented May 23, 2017

k8s-github-robot commented May 23, 2017

kubectl: fix deprecation warning bug #46062

kubectl: fix deprecation warning bug #46062

Conversation

alexandercampbell commented May 18, 2017 • edited

k8s-ci-robot commented May 18, 2017

k8s-ci-robot commented May 18, 2017

alexandercampbell commented May 19, 2017

pwittrock commented May 19, 2017

pwittrock commented May 19, 2017

pwittrock commented May 19, 2017

alexandercampbell commented May 19, 2017

droot commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock commented May 19, 2017

alexandercampbell commented May 19, 2017

droot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 22, 2017 • edited

alexandercampbell commented May 22, 2017

pwittrock commented May 23, 2017

pwittrock commented May 23, 2017

k8s-github-robot commented May 23, 2017

k8s-github-robot commented May 23, 2017

alexandercampbell commented May 18, 2017 •

edited

k8s-ci-robot commented May 22, 2017 •

edited