/origin

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

UPSTREAM: 17886: pod log location must validate container if provided #6113

Merged
merged 2 commits into from Nov 29, 2015

Conversation

Reviewers
No reviews
Assignees

@liggitt liggitt

Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@fabianofranz @liggitt @openshift-bot @smarterclayton @sdodson
@fabianofranz
@fabianofranz

This comment has been minimized.

Show comment
Hide comment
@fabianofranz

fabianofranz Nov 27, 2015

Member

Upstream PR: kubernetes/kubernetes#17886

@ncdc not sure if it's you, but ptal (here and upstream).
Member

fabianofranz commented Nov 27, 2015

Upstream PR: kubernetes/kubernetes#17886

@ncdc not sure if it's you, but ptal (here and upstream).
Godeps/_workspace/src/k8s.io/kubernetes/pkg/registry/pod/strategy.go Outdated
@@ -235,6 +235,7 @@ func LogLocation(getter ResourceGetter, connInfo client.ConnectionInfoGetter, ct
}
// Try to figure out a container
// If a container were provided, it must be valid

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Nov 27, 2015

Contributor

s/were/was/
Copy link
@liggitt

liggitt Nov 27, 2015

Contributor

s/were/was/

This comment has been minimized.

Show comment
Hide comment
@fabianofranz

fabianofranz Nov 27, 2015

Member

fixed
Copy link
@fabianofranz

fabianofranz Nov 27, 2015

Member

fixed
@fabianofranz

This comment has been minimized.

Show comment
Hide comment
@fabianofranz

fabianofranz Nov 27, 2015

Member

[test]
Member

fabianofranz commented Nov 27, 2015

[test]

@liggitt liggitt self-assigned this Nov 27, 2015

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Nov 28, 2015

Contributor 
--- FAIL: TestRegistryResourceLocation (0.00s)
    rest_test.go:73: Status: Complete Expected Location: https://foo-host:12345/containerLogs/default/running-build/foo-container, Got https://foo-host:12345/containerLogs/default/foo-pod/foo-container
    rest_test.go:73: Status: Failed Expected Location: https://foo-host:12345/containerLogs/default/running-build/foo-container, Got https://foo-host:12345/containerLogs/default/foo-pod/foo-container
    rest_test.go:73: Status: Running Expected Location: https://foo-host:12345/containerLogs/default/running-build/foo-container, Got https://foo-host:12345/containerLogs/default/foo-pod/foo-container
Contributor

liggitt commented Nov 28, 2015 

--- FAIL: TestRegistryResourceLocation (0.00s)
    rest_test.go:73: Status: Complete Expected Location: https://foo-host:12345/containerLogs/default/running-build/foo-container, Got https://foo-host:12345/containerLogs/default/foo-pod/foo-container
    rest_test.go:73: Status: Failed Expected Location: https://foo-host:12345/containerLogs/default/running-build/foo-container, Got https://foo-host:12345/containerLogs/default/foo-pod/foo-container
    rest_test.go:73: Status: Running Expected Location: https://foo-host:12345/containerLogs/default/running-build/foo-container, Got https://foo-host:12345/containerLogs/default/foo-pod/foo-container
@fabianofranz

This comment has been minimized.

Show comment
Hide comment
@fabianofranz

fabianofranz Nov 28, 2015

Member

@liggitt Seems just the mock, since we are now using the actual name from the rest api object, the mocks have to honour it. Fixed, let's see how it goes.
Member

fabianofranz commented Nov 28, 2015

@liggitt Seems just the mock, since we are now using the actual name from the rest api object, the mocks have to honour it. Fixed, let's see how it goes.
@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Nov 28, 2015

Contributor

Yup, split origin changes into their own commit
Contributor

liggitt commented Nov 28, 2015

Yup, split origin changes into their own commit
@fabianofranz

This comment has been minimized.

Show comment
Hide comment
@fabianofranz

fabianofranz Nov 28, 2015

Member

@liggitt of course, missed that. Amending. Done.
Member

fabianofranz commented Nov 28, 2015

@liggitt of course, missed that. Amending. Done.
@openshift-bot

This comment has been minimized.

Show comment
Hide comment
@openshift-bot

openshift-bot Nov 28, 2015

Contributor

Evaluated for origin test up to 9158aa1
Contributor

openshift-bot commented Nov 28, 2015

Evaluated for origin test up to 9158aa1
@openshift-bot

This comment has been minimized.

Show comment
Hide comment
@openshift-bot

openshift-bot Nov 28, 2015

Contributor

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

openshift-bot commented Nov 28, 2015

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

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Nov 29, 2015

Member

I will cut 1.1.0.1 with this once merged.
Member

smarterclayton commented Nov 29, 2015

I will cut 1.1.0.1 with this once merged.
@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Nov 29, 2015

Contributor

Lgtm, [merge]
Contributor

liggitt commented Nov 29, 2015

Lgtm, [merge]
@openshift-bot

This comment has been minimized.

Show comment
Hide comment
@openshift-bot

openshift-bot Nov 29, 2015

Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4177/) (Image: devenv-rhel7_2827)
Contributor

openshift-bot commented Nov 29, 2015

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4177/) (Image: devenv-rhel7_2827)
@openshift-bot

This comment has been minimized.

Show comment
Hide comment
@openshift-bot

openshift-bot Nov 29, 2015

Contributor

Evaluated for origin merge up to 9158aa1
Contributor

openshift-bot commented Nov 29, 2015

Evaluated for origin merge up to 9158aa1

openshift-bot added a commit that referenced this pull request Nov 29, 2015

@openshift-bot
Merge pull request #6113 from fabianofranz/bugs_1286094 
Merged by openshift-bot
4b1afe5

@openshift-bot openshift-bot merged commit 4b1afe5 into openshift:master Nov 29, 2015
3 checks passed

3 checks passed

continuous-integration/openshift-jenkins/merge Passed
Details
continuous-integration/openshift-jenkins/test Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sdodson

This comment has been minimized.

Show comment
Hide comment
@sdodson

sdodson Dec 1, 2015

Member

I will cut 1.1.0.1 with this once merged.

@smarterclayton This still happening?
Member

sdodson commented Dec 1, 2015

I will cut 1.1.0.1 with this once merged.

@smarterclayton This still happening?
@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Dec 1, 2015

Contributor

Should also pick up #6129
Contributor

liggitt commented Dec 1, 2015

Should also pick up #6129
@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Dec 1, 2015

Member

Once Jordan tells me to

On Dec 1, 2015, at 10:50 AM, Jordan Liggitt notifications@github.com
wrote:

Should also pick up #6129 #6129

—
Reply to this email directly or view it on GitHub
#6113 (comment).
Member

smarterclayton commented Dec 1, 2015

Once Jordan tells me to

On Dec 1, 2015, at 10:50 AM, Jordan Liggitt notifications@github.com
wrote:

Should also pick up #6129 #6129

—
Reply to this email directly or view it on GitHub
#6113 (comment).
@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Dec 1, 2015

Contributor

@smarterclayton go ahead (either from master or from ba7f510)
Contributor

liggitt commented Dec 1, 2015

@smarterclayton go ahead (either from master or from ba7f510)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment