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

openshift-diagnostics: package as oc alias #18149

Conversation

sosiouxme
Copy link
Member

Makes openshift-diagnostics a symlink to oc (like kubectl) so that
it can be packaged inside origin pods to be run from diagnostics.

fixes #18141

Makes `openshift-diagnostics` a symlink to `oc` (like `kubectl`) so that
it can be packaged inside origin pods to be run from diagnostics.

fixes openshift#18141
@sosiouxme sosiouxme requested review from eparis and deads2k January 17, 2018 21:24
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 17, 2018
@sosiouxme
Copy link
Member Author

sosiouxme commented Jan 17, 2018

This is the approach I personally prefer for openshift-diagnostics, just because it doesn't require a whole separate binary for this tiny function. But I'm not stuck on it, #18145 could be fine too. Someone tell me which way to go :)

Looks like the bot assigned the world on this one. That wasn't me.

@bparees bparees removed their request for review January 17, 2018 22:17
@sosiouxme
Copy link
Member Author

/retest

@sosiouxme sosiouxme added kind/bug Categorizes issue or PR as related to a bug. kind/delivery-blocker labels Jan 18, 2018
@juanvallejo
Copy link
Contributor

This is the approach I personally prefer for openshift-diagnostics, just because it doesn't require a whole separate binary for this tiny function.

I agree with this approach as well.
Changes lgtm from a cli perspective.

/lgtm

cc @deads2k

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

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: juanvallejo, sosiouxme
We suggest the following additional approver: liggitt

Assign the PR to them by writing /assign @liggitt in a comment when ready.

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

@deads2k
Copy link
Contributor

deads2k commented Jan 18, 2018

I'm -1 on this.

Aliases and polymorphic commands have not served us well in the past. Why can't you be built separately or even be a different image. I don't see a strong reason to couple our diagnostics to the same release schedule as oc. Especially as oc, kubectl, and ktl become more independent from the server bits.

@sosiouxme
Copy link
Member Author

Why can't you be built separately or even be a different image.

The binary can be built separately as it is now; that's the other PR. It just seems like a waste of space ( >100MB ) when everything it needs is already in the oc binary except the command definition.

There are a number of benefits to reusing the existing image. First, no need to build, ship, track a separate RPM and separate image (which is no small job for RCM, security team, etc). Second, the image is likely to be present when the diagnostic calls for it (less user time spent waiting for it to pull; the whole reason we went with the deployer image). I think there would have to be a pretty compelling argument for the overhead to separate out this tiny function.

I don't see a strong reason to couple our diagnostics to the same release schedule as oc.

Diagnostics are in oc already. This is just another diagnostics client running in the pod. I don't see why we would want those on a separate release schedule.

Especially as oc, kubectl, and ktl become more independent from the server bits.

Not sure what you mean... these aren't server bits either.

@sosiouxme
Copy link
Member Author

In fact... since this command just runs some "special" diagnostics, I could do away with it and unify these with the existing diagnostics command instead. Just pull in #17773 (which lacks only an approver) and add hidden oc adm diagnostics subcommands for network and pod diagnostics. @deads2k would that be better?

@deads2k
Copy link
Contributor

deads2k commented Jan 19, 2018

and add hidden oc adm diagnostics subcommands for network and pod diagnostics. @deads2k would that be better?

Yeah. I can go with subcommands under oc adm.

@sosiouxme
Copy link
Member Author

closed in favor of #18186

@sosiouxme sosiouxme closed this Jan 24, 2018
@sosiouxme sosiouxme deleted the 20180117-openshift-diagnostics-link branch January 24, 2018 19:40
openshift-merge-robot added a commit that referenced this pull request Jan 25, 2018
…cs-unify

Automatic merge from submit-queue.

openshift-diagnostics => diagnostics subcommands

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

openshift-diagnostics binary missing from origin images
4 participants