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.

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

UPSTREAM: 52440: add --dry-run option -> oadm <drain,cordon,uncordon> #16333

Merged
merged 2 commits into from
Oct 17, 2017
Merged

UPSTREAM: 52440: add --dry-run option -> oadm <drain,cordon,uncordon> #16333

merged 2 commits into from
Oct 17, 2017

Conversation

juanvallejo
Copy link
Contributor

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1491346

Adds a --dry-run flag to oc adm <cordon, uncordon, drain>

cc @openshift/cli-review

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 13, 2017
@juanvallejo juanvallejo force-pushed the jvallejo/add-dry-run-flag-oadm-drain branch from 48b1818 to 1d7e0cd Compare September 13, 2017 19:23
@juanvallejo juanvallejo changed the title add --dry-run option -> oadm <drain,cordon,uncordon> UPSTREAM: 52440: add --dry-run option -> oadm <drain,cordon,uncordon> Sep 13, 2017
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 13, 2017
@juanvallejo
Copy link
Contributor Author

/test cmd

2 similar comments
@juanvallejo
Copy link
Contributor Author

/test cmd

@juanvallejo
Copy link
Contributor Author

/test cmd

@juanvallejo
Copy link
Contributor Author

cc @fabianofranz

@fabianofranz
Copy link
Member

/lgtm
/approve https://bugzilla.redhat.com/show_bug.cgi?id=1491346

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2017
@juanvallejo
Copy link
Contributor Author

@fabianofranz or @soltysh mind re-adding /approve tag? thanks!

@soltysh
Copy link
Member

soltysh commented Sep 26, 2017

@juanvallejo from what I see this is different than the current state of upstream pr, mind updating it?

@juanvallejo
Copy link
Contributor Author

@soltysh

from what I see this is different than the current state of upstream pr, mind updating it?

it appears that RunCordonOrUncordon was updated upstream in patch db120eb. Will go ahead and bring ours up to date

@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2017
@juanvallejo juanvallejo force-pushed the jvallejo/add-dry-run-flag-oadm-drain branch from f1259e5 to fe989a4 Compare September 27, 2017 21:24
@soltysh
Copy link
Member

soltysh commented Sep 28, 2017

it appears that RunCordonOrUncordon was updated upstream in patch db120eb. Will go ahead and bring ours up to date

I can't be done the way you did it, track the PR that introduced that commit (it's kubernetes/kubernetes#48082 actually) and if it's relevant for us pick it. Otherwise just leave a comment and don't.

@juanvallejo
Copy link
Contributor Author

@soltysh the PR that introduced the commit makes use of the "k8s.io/api dep, which we don't have. I can leave a comment stating this, and revert the last commit on this PR. Unless it'd be better to bring in that dependency as part of this patch, wdyt?

@soltysh
Copy link
Member

soltysh commented Oct 3, 2017

@juanvallejo no, we don't have k8s.io/api because that was extracted from regular k8s.io/kubernetes/pkg/api as part of 1.8 work. Just replace the bits accordingly and drop the pieces we don't care.

@juanvallejo juanvallejo force-pushed the jvallejo/add-dry-run-flag-oadm-drain branch from fe989a4 to 0af44ff Compare October 3, 2017 17:00
@juanvallejo
Copy link
Contributor Author

@soltysh @fabianofranz upstream pr has merged, wondering if you could give this one more pass?

@juanvallejo
Copy link
Contributor Author

@soltysh @fabianofranz friendly ping

@openshift-merge-robot openshift-merge-robot added the vendor-update Touching vendor dir or related files label Oct 14, 2017
@fabianofranz
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 16, 2017
@juanvallejo
Copy link
Contributor Author

@soltysh wondering if there is any further feedback? Thx.

@soltysh
Copy link
Member

soltysh commented Oct 17, 2017

@juanvallejo squash the upstream changes into a single commit and you need to ping @deads2k for approval. Overall this LGTM.

@deads2k
Copy link
Contributor

deads2k commented Oct 17, 2017

/approve

Only bugs are merging at the moment. @fabianofranz can label if necessary.

@juanvallejo juanvallejo force-pushed the jvallejo/add-dry-run-flag-oadm-drain branch from 0af44ff to 7033a5f Compare October 17, 2017 17:30
@openshift-merge-robot openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm Indicates that a PR is ready to be merged. labels Oct 17, 2017
@juanvallejo
Copy link
Contributor Author

@soltysh thanks, upstream commits squashed

@fabianofranz
Copy link
Member

/lgtm
/kind bug

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. kind/bug Categorizes issue or PR as related to a bug. labels Oct 17, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit 80f9cb3 into openshift:master Oct 17, 2017
@juanvallejo juanvallejo deleted the jvallejo/add-dry-run-flag-oadm-drain branch October 18, 2017 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soltysh soltysh soltysh approved these changes

Assignees

@fabianofranz fabianofranz

@soltysh soltysh

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. queue/fix size/L Denotes a PR that changes 100-499 lines, ignoring generated files. vendor-update Touching vendor dir or related files
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@juanvallejo @fabianofranz @soltysh @deads2k @openshift-merge-robot @openshift-ci-robot