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

fix accessReview integration tests #3044

Closed
wants to merge 3 commits into from
Closed

fix accessReview integration tests #3044

wants to merge 3 commits into from

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Jun 10, 2015

Fixes #2899

Turns out it was just bad tests. The empty namespace used for the cluster requests works fine.

@liggitt

@deads2k deads2k force-pushed the fix-review-tests branch from da5a00b to 36ef31a Compare June 10, 2015 15:23
@deads2k
Copy link
Contributor Author

deads2k commented Jun 10, 2015

now with upstream patch.

@liggitt
Copy link
Contributor

liggitt commented Jun 10, 2015

[test]

liggitt
liggitt reviewed Jun 10, 2015
View reviewed changes
test/integration/authorization_test.go
@@ -159,7 +159,7 @@ func (test resourceAccessReviewTest) run(t *testing.T) {
}
}

if reflect.DeepEqual(actualResponse, test.response) {
if !reflect.DeepEqual(actualResponse.Users.List(), test.response.Users.List()) || !reflect.DeepEqual(actualResponse.Groups.List(), test.response.Groups.List()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you do the message prefix compare here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you do the message prefix compare here as well?

Added namespace. There is no message.

@openshift-bot
Copy link
Contributor

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

@deads2k deads2k force-pushed the fix-review-tests branch from 36ef31a to 4e8bedc Compare June 10, 2015 18:59
@openshift-bot
Copy link
Contributor

Evaluated for origin up to 4e8bedc

@liggitt
Copy link
Contributor

liggitt commented Jun 10, 2015 

--- FAIL: TestBootstrapPolicySelfSubjectAccessReviews-2 (7.66s)
    authorization_test.go:295: : from review
            &api.SubjectAccessReview{TypeMeta:api.TypeMeta{Kind:"", APIVersion:""}, Verb:"create", Resource:"policybindings", User:"", Groups:util.StringSet(nil), Content:runtime.EmbeddedObject{Object:runtime.Object(nil)}, ResourceName:""}
        expected
            &api.SubjectAccessReviewResponse{TypeMeta:api.TypeMeta{Kind:"", APIVersion:""}, Namespace:"openshift", Allowed:false, Reason:"denied by default"}
        got
            &api.SubjectAccessReviewResponse{TypeMeta:api.TypeMeta{Kind:"", APIVersion:""}, Namespace:"openshift", Allowed:false, Reason:"User \"valerie\" cannot create policybindings in project \"openshift\""}
    authorization_test.go:284: : expected
            cannot 
        got
            User "valerie" cannot create subjectaccessreviews in project "openshift"
FAIL

@liggitt
Copy link
Contributor

liggitt commented Jun 10, 2015

sorry, I didn't realize other files were using the subjectAccessReviewTest

liggitt
liggitt reviewed Jun 10, 2015
View reviewed changes
Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/apiserver/handlers.go
@@ -340,19 +340,6 @@ func (r *APIRequestInfoResolver) GetAPIRequestInfo(req *http.Request) (APIReques
currentParts = currentParts[2:]
}
}
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

I kept the else and set to this in my upstream PR:

requestInfo.Namespace = api.NamespaceAll

@liggitt liggitt self-assigned this Jun 10, 2015
@liggitt
Copy link
Contributor

liggitt commented Jun 10, 2015

Upstream PR at kubernetes/kubernetes#9600

@liggitt
Copy link
Contributor

liggitt commented Jun 10, 2015

With those two chunks removed, I got a clean unit and integration test pass with upstream. Once we get a clean test run, this should go in. @smarterclayton approve?

@smarterclayton
Copy link
Contributor 

11e5-a3d5-22000b3b0557", Groups:[]string(nil)}
--- FAIL: TestBootstrapPolicySelfSubjectAccessReviews-2 (7.66s)
    authorization_test.go:295: : from review
            &api.SubjectAccessReview{TypeMeta:api.TypeMeta{Kind:"", APIVersion:""}, Verb:"create", Resource:"policybindings", User:"", Groups:util.StringSet(nil), Content:runtime.EmbeddedObject{Object:runtime.Object(nil)}, ResourceName:""}
        expected
            &api.SubjectAccessReviewResponse{TypeMeta:api.TypeMeta{Kind:"", APIVersion:""}, Namespace:"openshift", Allowed:false, Reason:"denied by default"}
        got
            &api.SubjectAccessReviewResponse{TypeMeta:api.TypeMeta{Kind:"", APIVersion:""}, Namespace:"openshift", Allowed:false, Reason:"User \"valerie\" cannot create policybindings in project \"openshift\""}
    authorization_test.go:284: : expected
            cannot 
        got
            User "valerie" cannot create subjectaccessreviews in project "openshift"
FAIL

@liggitt
Copy link
Contributor

liggitt commented Jun 11, 2015

right, hasn't been updated since #3044 (comment)

@smarterclayton smarterclayton modified the milestone: 1.0.0 Jun 11, 2015
@liggitt liggitt mentioned this pull request Jun 11, 2015
Merged
@liggitt
Copy link
Contributor

liggitt commented Jun 11, 2015

opened #3082 with fix for failing test

@deads2k
Copy link
Contributor Author

deads2k commented Jun 11, 2015

@liggitt's update merged.

@deads2k deads2k closed this Jun 11, 2015
@deads2k deads2k deleted the fix-review-tests branch June 11, 2015 14:05
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
1.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@deads2k @liggitt @openshift-bot @smarterclayton