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

Make kubectl label and annotate more consistent #34199

Merged
merged 1 commit into from
Oct 8, 2016

Conversation

asalkeld
Copy link

@asalkeld asalkeld commented Oct 6, 2016

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:

  • 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.

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:

kubectl annotate now supports --dry-run

This change is Reviewable

@asalkeld
Copy link
Author

asalkeld commented Oct 6, 2016

@deads2k as requested...

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE Node e2e failed for commit ff855c05f6f3ca39ed7aa0fb1691a720a1abbe1b. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit ff855c05f6f3ca39ed7aa0fb1691a720a1abbe1b. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Oct 6, 2016
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.
@j3ffml j3ffml added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Oct 7, 2016
@j3ffml
Copy link
Contributor

j3ffml commented Oct 7, 2016

:lgtm:


Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


pkg/kubectl/cmd/annotate.go, line 39 at r1 (raw file):

// AnnotateOptions have the data required to perform the annotate operation
type AnnotateOptions struct {
  // Filename options

This comment is unnecessaryf (same for the one in label.go).


Comments from Reviewable

@asalkeld
Copy link
Author

asalkeld commented Oct 7, 2016

Review status: all files reviewed at latest revision, 1 unresolved discussion.


pkg/kubectl/cmd/annotate.go, line 39 at r1 (raw file):

Previously, jlowdermilk (Jeff Dempsey) wrote…

This comment is unnecessaryf (same for the one in label.go).

Yeah, Just wanted to separate the fields logically. Do you want me to remove before merging?

Comments from Reviewable

@deads2k
Copy link
Contributor

deads2k commented Oct 7, 2016

Yeah, Just wanted to separate the fields logically. Do you want me to remove before merging?

We can catch it in a sweep later. I think this change stands on its own. Nice job.

@deads2k deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 7, 2016
@@ -36,25 +36,26 @@ import (

// AnnotateOptions have the data required to perform the annotate operation
type AnnotateOptions struct {
// Filename options
resource.FilenameOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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-github-robot
Copy link

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

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 46c0099 into kubernetes:master Oct 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TechDebt: kubectl label and annotate have diverged and should more similar
6 participants