Skip to content

update resource selector - kubectl drain#54083

Merged
k8s-github-robot merged 2 commits intokubernetes:masterfrom
juanvallejo:jvallejo/update-resource-builder-cmd-drain
Oct 24, 2017
Merged

update resource selector - kubectl drain#54083
k8s-github-robot merged 2 commits intokubernetes:masterfrom
juanvallejo:jvallejo/update-resource-builder-cmd-drain

Conversation

@juanvallejo
Copy link
Copy Markdown
Contributor

@juanvallejo juanvallejo commented Oct 17, 2017

Followup to #52917

Release note:

NONE

Updates resource builder in cmd/drain.go to parse resource args similar to other commands.

cc @liggitt

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 17, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 17, 2017
@juanvallejo juanvallejo mentioned this pull request Oct 17, 2017
Closed
1 task
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Oct 17, 2017

Hi @juanvallejo. 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 /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.

I understand the commands that are listed here.

Details

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 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 Oct 17, 2017
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Oct 17, 2017

@juanvallejo: GitHub didn't allow me to assign the following users: liggit.

Note that only kubernetes members can be assigned.

Details

In response to this:

/assign liggit

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.

@juanvallejo
Copy link
Copy Markdown
Contributor Author

juanvallejo commented Oct 17, 2017

/assign liggitt

@liggitt
Copy link
Copy Markdown
Member

liggitt commented Oct 17, 2017

/ok-to-test
/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 17, 2017
@liggitt
Copy link
Copy Markdown
Member

liggitt commented Oct 18, 2017 •
edited
Loading

/lgtm cancel

causes failures in some invocations (see run_cluster_management_tests in test-cmd):

      error: You must provide one or more resources by argument or filename.
Example resource specifications include:
   '-f rsrc.yaml'
   '--filename=rsrc.json'
   '<resource> <name>'
   '<resource>'

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 18, 2017
@juanvallejo
Copy link
Copy Markdown
Contributor Author

juanvallejo commented Oct 18, 2017

@liggitt thanks, had forgotten to update one of the cmd-tests for cordon/uncordon which previously expected all nodes to be selected when an empty selector value was passed

liggitt
liggitt reviewed Oct 18, 2017
View reviewed changes
hack/make-rules/test-cmd-util.sh Outdated
Copy link
Copy Markdown
Member

@liggitt liggitt Oct 18, 2017

Choose a reason for hiding this comment

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

do you have a negative selector test? (pass a selector that doesn't match anything)

Copy link
Copy Markdown
Contributor Author

@juanvallejo juanvallejo Oct 18, 2017

Choose a reason for hiding this comment

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

Will add one, thanks

@juanvallejo juanvallejo force-pushed the jvallejo/update-resource-builder-cmd-drain branch from 28e3de9 to 5f48d45 Compare October 19, 2017 14:01
@juanvallejo
Copy link
Copy Markdown
Contributor Author

juanvallejo commented Oct 19, 2017

@liggitt thanks for the feedback, added a negative selector test

@juanvallejo juanvallejo mentioned this pull request Oct 19, 2017
Merged
@juanvallejo juanvallejo force-pushed the jvallejo/update-resource-builder-cmd-drain branch from 5f48d45 to bb86297 Compare October 19, 2017 14:13
@juanvallejo
Copy link
Copy Markdown
Contributor Author

juanvallejo commented Oct 20, 2017

@liggitt friendly ping

@liggitt
Copy link
Copy Markdown
Member

liggitt commented Oct 21, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 21, 2017
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Oct 24, 2017 •
edited
Loading

@juanvallejo: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce-bazel c8887a1dad4e3c99f9bc062b437513fe706cdb34 link /test pull-kubernetes-e2e-gce-bazel
pull-kubernetes-e2e-gce-etcd3 c8887a1dad4e3c99f9bc062b437513fe706cdb34 link /test pull-kubernetes-e2e-gce-etcd3

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.

Details

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.

@juanvallejo
Copy link
Copy Markdown
Contributor Author

juanvallejo commented Oct 24, 2017

/retest

@fabianofranz
Copy link
Copy Markdown
Contributor

fabianofranz commented Oct 24, 2017

/approve

@fabianofranz
Copy link
Copy Markdown
Contributor

fabianofranz commented Oct 24, 2017

/approve no-issue

@k8s-github-robot
Copy link
Copy Markdown

k8s-github-robot commented Oct 24, 2017

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabianofranz, juanvallejo, liggitt

Associated issue requirement bypassed by: fabianofranz

The full list of commands accepted by this bot can be found here.

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

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 k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 24, 2017
@k8s-github-robot
Copy link
Copy Markdown

k8s-github-robot commented Oct 24, 2017

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link
Copy Markdown

k8s-github-robot commented Oct 24, 2017

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 2ad7d6f into kubernetes:master Oct 24, 2017
@juanvallejo juanvallejo deleted the jvallejo/update-resource-builder-cmd-drain branch October 24, 2017 22:29
@juanvallejo juanvallejo mentioned this pull request Dec 5, 2017
Merged
openshift-merge-robot added a commit to openshift/origin that referenced this pull request Dec 21, 2017
@openshift-merge-robot
Merge pull request #17616 from juanvallejo/jvallejo/add-selector-oc-d…
d8c46e4 
…rain

Automatic merge from submit-queue (batch tested with PRs 17072, 17616).

Add --selector, --pod-selector flags `oc adm drain`

Fixes #17554
Fixes #17563
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1525340

Picks kubernetes/kubernetes#52917, kubernetes/kubernetes#54083, and kubernetes/kubernetes#56864 to bring in `--selector` and `--pod-selector` flag support to `oc adm drain`.

cc @openshift/cli-review @deads2k @dustymabe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liggitt liggitt liggitt left review comments

Assignees

@sttts sttts

@liggitt liggitt

@ghodss ghodss

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. 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-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@juanvallejo @k8s-ci-robot @liggitt @fabianofranz @k8s-github-robot @sttts @ghodss