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 new command "kubectl set selector" #38966

Merged
merged 1 commit into from Dec 21, 2016
Merged

Add new command "kubectl set selector" #38966

merged 1 commit into from Dec 21, 2016

Conversation

0xmichalis
Copy link
Contributor

Continuation of #28949

@0xmichalis 0xmichalis added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Dec 19, 2016
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 19, 2016
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added kind/old-docs size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 19, 2016
@0xmichalis
Copy link
Contributor Author

@kubernetes/kubectl

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit 992baaa. Full PR test history.

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

@0xmichalis
Copy link
Contributor Author

@k8s-bot gce etcd3 e2e test this

@@ -147,9 +147,9 @@ type ClientAccessFactory interface {
// Returns a Printer for formatting objects of the given type or an error.
Printer(mapping *meta.RESTMapping, options kubectl.PrintOptions) (kubectl.ResourcePrinter, error)
// Pauser marks the object in the info as paused ie. it will not be reconciled by its controller.
Pauser(info *resource.Info) (bool, error)
Pauser(info *resource.Info) ([]byte, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Describe []byte in the Godoc here.

Copy link
Contributor

@smarterclayton smarterclayton left a comment

Choose a reason for hiding this comment

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

One comment, looks good to me. Might be good for someone else to cast a critical eye.

// CalculatePatches calls the mutation function on each provided info object, and generates a strategic merge patch for
// the changes in the object. Encoder must be able to encode the info into the appropriate destination type. If mutateFn
// returns false, the object is not included in the final list of patches.
func CalculatePatches(infos []*resource.Info, encoder runtime.Encoder, mutateFn func(*resource.Info) (bool, error)) []*Patch {
func CalculatePatches(infos []*resource.Info, encoder runtime.Encoder, mutateFn func(*resource.Info) ([]byte, error)) []*Patch {
Copy link
Contributor

Choose a reason for hiding this comment

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

doc is now wrong. update with what the mutateFn returns now. Also, promote it to a real type and doc it there.

// SelectorOptions is the start of the data required to perform the operation. As new fields are added, add them here instead of
// referencing the cmd.Flags()
type SelectorOptions struct {
fnOptions 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.

fileOptions. fn looks too much line function.

cmdutil.AddFilenameOptionFlags(cmd, &options.fnOptions, usage)
cmdutil.AddDryRunFlag(cmd)
cmdutil.AddRecordFlag(cmd)
cmdutil.AddInclude3rdPartyFlags(cmd)
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't needed anymore.

@deads2k
Copy link
Contributor

deads2k commented Dec 20, 2016

minor comments, then lgtm.

@0xmichalis
Copy link
Contributor Author

Comments addressed, applying lgtm

@0xmichalis 0xmichalis added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 21, 2016
@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 b77e076 into kubernetes:master Dec 21, 2016
@0xmichalis 0xmichalis deleted the set-selector branch December 21, 2016 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants