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

oc debug: fix crash on attach #20697

Merged
merged 2 commits into from
Aug 23, 2018

Conversation

rphillips
Copy link
Contributor

o.AttachFunc is nil since we use our own custom oc debug command.
Attaching the defaultAttachFunc if nil fixes the crash and gives a
prompt.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1618522

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 20, 2018
@rphillips rphillips force-pushed the fix/1618522 branch 2 times, most recently from 6803cb7 to 5dc7505 Compare August 20, 2018 16:21
@rphillips
Copy link
Contributor Author

/test cmd

@sjenning
Copy link
Contributor

Upstream pick that introduced this c7e00d1

oc debug calls directly into the AttachOptions.Run() and does not call Complete() which is what initializes o.AttachFunc to defaultAttachFunc in kubectl attach upstream. Thus upstream does not suffer this with kubectl attach but we do with oc debug.

LGTM but want CLI to look at this
/cc @sttts @juanvallejo

@rphillips
Copy link
Contributor Author

rphillips commented Aug 20, 2018

We could improve the upstream code to avoid the carry patch by exporting the defaultAttachFunc. The initialization of AttachFunc is odd as well. By initializing the upstream AttachFunc in the New... constructors the code would be setup from the initial construction to be overridden by our debug implementation.

@rphillips
Copy link
Contributor Author

Upstream PR: kubernetes/kubernetes#67615

@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 20, 2018
@sjenning
Copy link
Contributor

/hold
on different route proposed upstream

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 20, 2018
@juanvallejo
Copy link
Contributor

lgtm provided upstream PR is approved

@juanvallejo
Copy link
Contributor

juanvallejo commented Aug 21, 2018

Also, please split your commit into two. First one should be "UPSTREAM: 67615: attach: Move the AttachFunc default function to the initializer" and be up to date with upstream changes, second one should contain Origin changes

@soltysh soltysh self-assigned this Aug 22, 2018
Copy link
Member

@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.

Pick the upstream changes as a separate UPSTREAM commit and in the another one make the oc specific changes. I just approved uptream PR, so make sure to update that as well.

…initializer

Fixes a partially constructed AttachOptions
Refactors to pick up the initializer for defaultAttachFunc

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1618522
@rphillips
Copy link
Contributor Author

@soltysh rebased and refactored... good to go for another review.

@rphillips
Copy link
Contributor Author

/test cmd
/test integration

Copy link
Member

@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

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 23, 2018
@soltysh
Copy link
Member

soltysh commented Aug 23, 2018

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 23, 2018
@soltysh soltysh added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 23, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: rphillips, soltysh

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@soltysh
Copy link
Member

soltysh commented Aug 23, 2018

Upstream merged, this is properly updated, approving.

@openshift-merge-robot openshift-merge-robot merged commit 64ac00d into openshift:master Aug 23, 2018
@openshift-ci-robot
Copy link

@rphillips: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/gcp b051e60 link /test gcp

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants