Make kubectl label and annotate more consistent #34199
Conversation
|
@deads2k as requested... |
|
Jenkins GCE Node e2e failed for commit ff855c05f6f3ca39ed7aa0fb1691a720a1abbe1b. Full PR test history. The magic incantation to run this job again is |
|
Jenkins verification failed for commit ff855c05f6f3ca39ed7aa0fb1691a720a1abbe1b. Full PR test history. The magic incantation to run this job again is |
k8s-github-robot
added
size/L
Some of the main changes: - add dryrun to annotate (can push this in a different PR if requested) - use Complete(), Validate() and RunX() - don't place dynamic variables in the options (only user options and args) - call the NewBuilder() in the Run function. You can now do diff between these files and they are as identical as possible.
asalkeld
force-pushed
the
annotate-label
branch
from
ff855c0 to
4a09ded
Compare
j3ffml
added
release-note
|
Reviewed 4 of 4 files at r1. pkg/kubectl/cmd/annotate.go, line 39 at r1 (raw file):
This comment is unnecessaryf (same for the one in label.go). Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 1 unresolved discussion. pkg/kubectl/cmd/annotate.go, line 39 at r1 (raw file):
|
We can catch it in a sweep later. I think this change stands on its own. Nice job. |
deads2k
added
the
lgtm
| @@ -36,25 +36,26 @@ import ( | |||
|
|
|||
| // AnnotateOptions have the data required to perform the annotate operation | |||
| type AnnotateOptions struct { | |||
| // Filename options | |||
| resource.FilenameOptions | |||
There was a problem hiding this comment.
I'd like to avoid anonymous inclusion where possible. The method aliasing seems to encourage bad habits (see Factory) and gain for non-serialized types isn't very significant. I won't block on it.
| local := cmdutil.GetFlagBool(cmd, "local") | ||
|
|
||
| // RunLabel does the work | ||
| func (o *LabelOptions) RunLabel(f *cmdutil.Factory, cmd *cobra.Command) error { |
There was a problem hiding this comment.
In the end state, we'd like the options struct's Run to stand on their own, completely divorced from cobra for re-use and delegation as well as testing. Something to keep in mind.
Issue ref: #7311
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue |
What this PR does / why we need it:
This makes the label and annotate cmd files more consistent which should help with code maintenance.
Some of the main changes:
Which issue this PR fixes
fixes #34151
Special notes for your reviewer:
Note: you can now diff the two files and the changes make sense.
Release note:
This change is