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

Make sure --record=false is acknowledged when passed to commands #28234

Merged

k8s-github-robot merged 1 commit into kubernetes:master from damemi:record-false-bugfix

Jul 18, 2016

Contributor

damemi commented Jun 29, 2016 •

edited by pwittrock

Loading

Change setting "kubectl --record=false" to stop updating the change-cause when a previous change-cause is found.

Ensures that when --record=false is explicity set that no ChangeCauseAnnotations are set on the object. Previously, if --record=true was used then all following actions triggered a ChangeCauseAnnotation even if --record=false was set, due to the prior ChangeCauseAnnotation existing.

Reference to bug report: https://bugzilla.redhat.com/show_bug.cgi?id=1351127

googlebot added the cla: yes label

k8s-bot commented Jun 29, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

4 similar comments

k8s-bot commented Jun 29, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot commented Jun 29, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot commented Jun 29, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot commented Jun 29, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

Member

ncdc commented Jun 29, 2016

cc @kubernetes/kubectl

k8s-github-robot assigned pwittrock

k8s-github-robot added size/XS release-note-label-needed labels

Member

ncdc commented Jun 29, 2016

ok to test

pwittrock reviewed

View reviewed changes

pkg/kubectl/cmd/util/helpers.go

    
            @@ -491,7 +491,7 @@ func ContainsChangeCause(info *resource.Info) bool {
          
              // ShouldRecord checks if we should record current change cause

              func ShouldRecord(cmd *cobra.Command, info *resource.Info) bool {

              	return GetRecordFlag(cmd) || ContainsChangeCause(info)

              	return GetRecordFlag(cmd) || (ContainsChangeCause(info) && !cmd.Flags().Changed("record"))

Member

pwittrock Jun 29, 2016

Please update the flag documentation to be more clear. Also please add a test for this.

Member

ixdy commented Jun 29, 2016

@k8s-bot node e2e test this please, issue #IGNORE

damemi force-pushed the record-false-bugfix branch from 193d13c to 8f4b911 Compare

June 30, 2016 20:11

k8s-github-robot added size/S and removed size/XS labels

Member

ixdy commented Jun 30, 2016

ok to test

Contributor Author

damemi commented Jul 1, 2016

It looks like the build failed due to #27272 and #28047

Member

pwittrock commented Jul 1, 2016

@k8s-bot node e2e test this issue: #28047

Contributor Author

damemi commented Jul 11, 2016

@pwittrock bump, is there anything else you need me to do with this? thanks

k8s-github-robot added the needs-rebase label

damemi force-pushed the record-false-bugfix branch 2 times, most recently from 5c9916b to 391f4c9 Compare

July 13, 2016 13:11

k8s-github-robot removed the needs-rebase label

Member

pwittrock commented Jul 14, 2016

@damemi Sorry I was OOO for the past 2 weeks.

Would you update the documentation for the --record flag in helpers.go to clarify what happens when it is unset. It currently just says

Record current kubectl command in the resource annotation.

This doesn't accurately cover the flag values.

Maybe:

Record current kubectl command in the resource annotation. If set to false, do not record the command. If set to true, record the command. If not set, default to updating the existing annotation value only if one already exists.

pwittrock added this to the v1.4 milestone

pwittrock added the priority/awaiting-more-evidence label

damemi force-pushed the record-false-bugfix branch from 391f4c9 to 146a3c4 Compare

July 14, 2016 19:12

Contributor Author

damemi commented Jul 14, 2016

@pwittrock that's fine, updated!

pwittrock reviewed

View reviewed changes

hack/make-rules/test-cmd.sh

    
                ### Do not record label change

                # Command

                kubectl label pods valid-pod no-record-change=true "${kube_flags[@]}" --record=false

Member

pwittrock Jul 14, 2016

nit: would you make sure the flags are given in a consistent order to make it easier to read?

pwittrock reviewed

View reviewed changes

hack/make-rules/test-cmd.sh

    
                # Post-condition: valid-pod's record annotation still contains command with --record=true

                kube::test::get_object_assert 'pod valid-pod' "{{range$annotations_field}}{{.}}:{{end}}" ".*--record=true.*"

Member

pwittrock Jul 14, 2016

nit: Delete the extra new lines

Member

pwittrock Jul 14, 2016

Please also add a test that the record is written if the flag is unspecified and the object has a previous record

Member

pwittrock commented Jul 14, 2016

@damemi Thanks. Just a couple comments on the test. Almost there.

damemi force-pushed the record-false-bugfix branch from 146a3c4 to a616266 Compare

July 15, 2016 14:44


          Make sure --record=false is acknowledged when passed to commands

cfbf7da

damemi force-pushed the record-false-bugfix branch from a616266 to cfbf7da Compare

July 15, 2016 15:24

k8s-bot commented Jul 15, 2016

GCE e2e build/test passed for commit cfbf7da.

pwittrock added release-note lgtm and removed release-note-label-needed labels

Member

pwittrock commented Jul 18, 2016

@damemi Thanks

k8s-github-robot commented Jul 18, 2016

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot commented Jul 18, 2016

GCE e2e build/test passed for commit cfbf7da.

k8s-github-robot commented Jul 18, 2016

Automatic merge from submit-queue

k8s-github-robot merged commit e72f6a8 into kubernetes:master

damemi mentioned this pull request

REQUEST: New membership for damemi kubernetes/org#860

Closed

6 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm priority/awaiting-more-evidence release-note size/S