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

openshift-diagnostics => diagnostics subcommands #18186

Merged
merged 1 commit into from
Jan 25, 2018
Merged

openshift-diagnostics => diagnostics subcommands #18186

merged 1 commit into from
Jan 25, 2018

Conversation

sosiouxme
Copy link
Member

@sosiouxme sosiouxme commented Jan 19, 2018 •
edited
Loading

Builds on commit from #17773 (diagnostics: individual parameters).
Removes openshift-diagnostics in favor of hidden oc adm diagnostics subcommands as proposed with #18149 (comment).

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1534513 and #18141

@sosiouxme sosiouxme added kind/bug Categorizes issue or PR as related to a bug. kind/delivery-blocker labels Jan 19, 2018
@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 19, 2018
@sosiouxme sosiouxme force-pushed the 20180117-openshift-diagnostics-unify branch 3 times, most recently from 3b62cc2 to 8746c7b Compare January 19, 2018 18:10
@sosiouxme
Copy link
Member Author

Fixed bug that e2e test uncovered. Should be good now.
Other test failure was a flake.

@sosiouxme
Copy link
Member Author

@deads2k any chance we could get this in today?

One more alternative: don't try to make these diagnostics subcommands. Instead, just create hidden oc adm inpod-diagnostics or similar. That wouldn't require pulling in the other PR.

@juanvallejo
Copy link
Contributor

/lgtm
from a cli perspective

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 19, 2018
@sosiouxme sosiouxme force-pushed the 20180117-openshift-diagnostics-unify branch from 8746c7b to 99e5c7f Compare January 19, 2018 21:27
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 19, 2018
@sosiouxme
Copy link
Member Author

Rebased and squashed.

@sosiouxme sosiouxme force-pushed the 20180117-openshift-diagnostics-unify branch from 99e5c7f to 7b8ab0f Compare January 19, 2018 21:47
@sosiouxme
Copy link
Member Author

https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/18186/test_pull_request_origin_extended_conformance_install/5951/ looks like #17882
/test extended_conformance_install

@sosiouxme
Copy link
Member Author

@juanvallejo and @deads2k this is just waiting for your lgtm and approval (respectively). Base PR #17773 was lgtm/approved but needed tweaks to pass tests; we can ask @soltysh to lgtm that again if you want to merge it separately, or do both at once via this PR. Just looking to proceed with this blocker however you think that's best.

@juanvallejo
Copy link
Contributor

@sosiouxme looks like completions were the only thing that changed in #17773? If so, @deads2k I'm fine with merging it in one go in this PR.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 22, 2018
@deads2k
Copy link
Contributor

deads2k commented Jan 23, 2018

I'm fine with it in principle. @soltysh has final approval.

@soltysh check the owners files and figure out which one you don't have rights to.

@sosiouxme
Copy link
Member Author

#17773 merged so this is really just the second commit.

@soltysh
Copy link
Contributor

soltysh commented Jan 24, 2018

@soltysh check the owners files and figure out which one you don't have rights to.

docs/man was one, but this PR touches much more dirs: hack, images, contrib which I don't need approval rights for oc itself.

soltysh
soltysh approved these changes Jan 24, 2018
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@soltysh soltysh added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 24, 2018
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2018
@sosiouxme
openshift-diagnostics => diagnostics subcommands
776865b 
Instead of building a separate `openshift-diagnostics` just turn its
subcommands into hidden subcommands on `oc adm diagnostics`.
Then use these within diagnostics pods.

fixes openshift#18141
fixes https://bugzilla.redhat.com/show_bug.cgi?id=1534513
@sosiouxme sosiouxme force-pushed the 20180117-openshift-diagnostics-unify branch from 7b8ab0f to 776865b Compare January 24, 2018 19:35
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2018
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 24, 2018
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2018
@stevekuznetsov stevekuznetsov removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 24, 2018
@stevekuznetsov
Copy link
Contributor

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 24, 2018
@sosiouxme
Copy link
Member Author

Rebased (should have seen that coming).

@stevekuznetsov
Copy link
Contributor

/lgtm
for rebase commit

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juanvallejo, soltysh, sosiouxme, stevekuznetsov

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

@sosiouxme sosiouxme mentioned this pull request Jan 24, 2018
Closed
@sosiouxme
Copy link
Member Author

Bad luck, https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/18186/test_pull_request_origin_extended_conformance_gce/14947/ hit a rare one -- #15059

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit 189d01b into openshift:master Jan 25, 2018
@sosiouxme sosiouxme deleted the 20180117-openshift-diagnostics-unify branch January 25, 2018 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juanvallejo juanvallejo juanvallejo approved these changes

@soltysh soltysh soltysh approved these changes

@deads2k deads2k Awaiting requested review from deads2k

@liggitt liggitt Awaiting requested review from liggitt

@pecameron pecameron Awaiting requested review from pecameron

@pravisankar pravisankar Awaiting requested review from pravisankar

@rajatchopra rajatchopra Awaiting requested review from rajatchopra

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

@stevekuznetsov stevekuznetsov Awaiting requested review from stevekuznetsov

Assignees

@soltysh soltysh

@juanvallejo juanvallejo

@stevekuznetsov stevekuznetsov

@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. kind/delivery-blocker lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@sosiouxme @juanvallejo @deads2k @soltysh @stevekuznetsov @openshift-ci-robot @openshift-bot @openshift-merge-robot