only resolve roles for bindings that matter #1231
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| for _, value := range policyBinding.RoleBindings { | ||
| ret = append(ret, value) | ||
| for _, roleBinding := range policyBinding.RoleBindings { | ||
| if doesApplyToUser(roleBinding.Users, roleBinding.Groups, user) { |
Contributor
There was a problem hiding this comment.
Please rename this "appliesToUser()" - this wording makes my brain hurt a bit
deads2k
force-pushed
the
deads-only-resolve-needed
branch
from
f186616 to
c136e4f
Compare
|
|
||
| if err = openshiftClient.Roles("master").Delete("view"); err != nil { | ||
| t.Errorf("unexpected error: %v", err) | ||
| } |
Contributor
There was a problem hiding this comment.
maybe try to run addValerie again and make sure you get an error when the role is invalid?
Contributor
|
suggestion for additional test, LGTM otherwise |
deads2k
force-pushed
the
deads-only-resolve-needed
branch
2 times, most recently
from
cb4f098 to
1dd9b78
Compare
deads2k
force-pushed
the
deads-only-resolve-needed
branch
from
1dd9b78 to
1d15a87
Compare
Contributor
Author
|
@liggitt Can you re-review changes to find_rules.go? Integration tests caught an insidious bug caused by our magic container (aka context). |
Contributor
|
yay tests. LGTM |
Contributor
Author
|
[merge] |
Contributor
Author
|
opened #1234 to tight method preconditions |
Contributor
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1122/) (Image: devenv-fedora_981) |
Contributor
|
Evaluated for origin up to 1d15a87 |
jboyd01
pushed a commit
to jboyd01/origin
that referenced
this pull request
Sep 20, 2017
…service-catalog/' changes from ae6b643caf..50e234de83 50e234de83 origin build: add origin tooling 092d7f8 Fix typos and resource names in walkthrough e2e logs (openshift#1237) d25bd11 Archive the old agenda doc, link to new one (openshift#1243) 6192d14 fix lint errors (openshift#1242) d103dad Fix lint errors and regenerate openapi (openshift#1238) e9328d3 Broker Relist (openshift#1183) b0f3222 Correct the reasons and messages set on the ready condition during async polling (openshift#1235) d2bb82f Re-enable the href checker (openshift#1232) 2c29654 Use feature gates in controller-manager (openshift#1231) 699eab9 switch build to go1.9 (openshift#1155) 7529ed8 broker resource secret authorization checking (openshift#1186) 50d9bdf v0.0.20 chart updates (openshift#1228) REVERT: ae6b643caf Use oc adm instead of oadm which might not exist in various installations. REVERT: 66a4eb2a2c Update instructions... will remove once documented elsewhere REVERT: 1b704d1530 replace build context setup with init containers REVERT: ee4df18c7f hack/lib: dedup os::util::host_platform and os::build::host_platform REVERT: 1cd6dfa998 origin: Switch out owners to Red Hatters REVERT: 664f4d318f Add instructions for syncing repos REVERT: 2f2cdd546b origin-build: delete files with colon in them REVERT: cdf8b12848 origin-build: don't build user-broker REVERT: ebfede9056 origin build: add _output to .gitignore REVERT: 55412c7e3d origin build: make build-go and build-cross work REVERT: 68c74ff4ae origin build: modify hard coded path REVERT: 3d41a217f6 origin build: add origin tooling git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog git-subtree-split: 50e234de836b5e7c9e3d7d763847b99a0f0ea500
jboyd01
pushed a commit
to jboyd01/origin
that referenced
this pull request
Sep 21, 2017
…service-catalog/' changes from ae6b643caf..06b897d198 06b897d198 origin build: add origin tooling 092d7f8 Fix typos and resource names in walkthrough e2e logs (openshift#1237) d25bd11 Archive the old agenda doc, link to new one (openshift#1243) 6192d14 fix lint errors (openshift#1242) d103dad Fix lint errors and regenerate openapi (openshift#1238) e9328d3 Broker Relist (openshift#1183) b0f3222 Correct the reasons and messages set on the ready condition during async polling (openshift#1235) d2bb82f Re-enable the href checker (openshift#1232) 2c29654 Use feature gates in controller-manager (openshift#1231) 699eab9 switch build to go1.9 (openshift#1155) 7529ed8 broker resource secret authorization checking (openshift#1186) 50d9bdf v0.0.20 chart updates (openshift#1228) REVERT: ae6b643caf Use oc adm instead of oadm which might not exist in various installations. REVERT: 66a4eb2a2c Update instructions... will remove once documented elsewhere REVERT: 1b704d1530 replace build context setup with init containers REVERT: ee4df18c7f hack/lib: dedup os::util::host_platform and os::build::host_platform REVERT: 1cd6dfa998 origin: Switch out owners to Red Hatters REVERT: 664f4d318f Add instructions for syncing repos REVERT: 2f2cdd546b origin-build: delete files with colon in them REVERT: cdf8b12848 origin-build: don't build user-broker REVERT: ebfede9056 origin build: add _output to .gitignore REVERT: 55412c7e3d origin build: make build-go and build-cross work REVERT: 68c74ff4ae origin build: modify hard coded path REVERT: 3d41a217f6 origin build: add origin tooling git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog git-subtree-split: 06b897d1988a5a3c035c5a971c15b97cbc732918
jpeeler
pushed a commit
to jpeeler/origin
that referenced
this pull request
Feb 1, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Only resolve roles for bindings that matter, so that we limit the scope of referential integrity problems between bindings and roles. Addresses the last comment in https://bugzilla.redhat.com/show_bug.cgi?id=1192310, but I'm going to see I can a separate defect for the different symptom.
@liggitt easy.