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

Bug 1261548 - oc run --attach support for DeploymentConfig #5271

Merged
merged 2 commits into from Oct 29, 2015
Merged

Bug 1261548 - oc run --attach support for DeploymentConfig #5271

merged 2 commits into from Oct 29, 2015

Conversation

fabianofranz
Copy link
Member

No description provided.

@fabianofranz
Copy link
Member Author

[test]

@liggitt liggitt self-assigned this Oct 21, 2015
deads2k
deads2k reviewed Oct 21, 2015
View reviewed changes
pkg/cmd/util/clientcmd/factory.go
}
switch t := object.(type) {
case *deployapi.DeploymentConfig:
var pods *api.PodList
Copy link
Contributor

Choose a reason for hiding this comment

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

For this var, do you intend to retrieve the deployer pod or the first pod of the first deployment's RC? As of now, the intent for oc logs dc/foo is to show the deployer pod's logs.

@Kargakis if you have opinions.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be consistent with oc logs dc/foo so I think returning the deployer pod here is the right thing to do.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not confident it should be the deployer pod. If you do oc run nginx --image=nginx --attach aren't you expecting to attach to the actual nginx pod? In fact you may not even know a deployment is involved in this oc run operation. @liggitt opinion?

Copy link
Contributor

Choose a reason for hiding this comment

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

@fabianofranz, had a closer look to oc run and I think you are right.

@fabianofranz fabianofranz force-pushed the bugs_1261548 branch 2 times, most recently from 373b619 to 48b66fc Compare October 22, 2015 20:11
@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/6193/)

@openshift-bot openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 22, 2015
@smarterclayton
Copy link
Contributor

Why do we want this?

@fabianofranz
Copy link
Member Author

@smarterclayton If you try for example oc run nginx --image=nginx --attach today you will get a "error: cannot attach to DeploymentConfig: not implemented".

That's because we override run to create a dc instead of a rc, but the upstream command doesn't know how to handle --attach for that type. So this exposed the attachable pod discovery in the factory so that other types can be supported, including deployment configs.

@fabianofranz fabianofranz changed the title [WIP] Bug 1261548 - oc run --attach support for DeploymentConfig Bug 1261548 - oc run --attach support for DeploymentConfig Oct 26, 2015
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2015
@fabianofranz
Copy link
Member Author

Other comments?

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 65775d3

@pweil- pweil- mentioned this pull request Oct 28, 2015
Closed
@smarterclayton
Copy link
Contributor

Open a follow up to make this logic cleaner (a condition that waits until there is a pod in the replication controller, in upstream pkg/client/conditions.go).

@smarterclayton
Copy link
Contributor

LGTM [merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/6193/) (Image: devenv-rhel7_2589)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 65775d3

openshift-bot pushed a commit that referenced this pull request Oct 29, 2015
Merge pull request #5271 from fabianofranz/bugs_1261548
8a312b2 
Merged by openshift-bot
@openshift-bot openshift-bot merged commit 8a312b2 into openshift:master Oct 29, 2015
@fabianofranz fabianofranz mentioned this pull request Oct 29, 2015
Closed
@fabianofranz
Copy link
Member Author

@smarterclayton thanks. added #5484

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@liggitt liggitt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@fabianofranz @openshift-bot @smarterclayton @0xmichalis @deads2k @liggitt