Update membership role filter to use new annotation #1172
Conversation
|
@enj fyi |
| ! _.startsWith(item.metadata.name, 'system:') && | ||
| ! _.startsWith(item.metadata.name, 'registry-') && | ||
| ! _.startsWith(item.metadata.name, 'self-'); | ||
| return !annotation(item, 'systemOnly'); |
There was a problem hiding this comment.
you need to actually check the content of the annotation and compare it to the string 'true' in theory someone could set the annotation and set it to false
There was a problem hiding this comment.
Right, & verified with @enj that is consistent with some other annotations. So only 'true' will be honored. Updated.
|
@benjaminapetersen have you run the latest origin images and made sure this works as expected in combination with that? |
|
@jwforres yup, built it this morn |
benjaminapetersen
force-pushed
the
role-filter-update
branch
from
09bbd2f to
d9d7eb7
Compare
|
@benjaminapetersen the travis failure is because your spec tests need to be updated |
benjaminapetersen
force-pushed
the
role-filter-update
branch
from
d9d7eb7 to
d1ba604
Compare
|
fixed |
| // ignored | ||
| {metadata: {name: 'admin'}}, | ||
| {metadata: {name: 'edit'}} | ||
| {metadata: {name: 'system-only-role', annotations: {'authorization.openshift.io/system-only': 'true'}}}, |
There was a problem hiding this comment.
Can you add one with the annotation set to false
There was a problem hiding this comment.
Yeah ill add a few more examples of things that wont actually pass the filter, just to be clear.
benjaminapetersen
force-pushed
the
role-filter-update
branch
from
d1ba604 to
4884f0c
Compare
|
Evaluated for origin web console test up to 4884f0c |
|
Origin Web Console Test Results: Running (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/971/) (Base Commit: 5d5983c) |
|
[merge] |
|
Evaluated for origin web console merge up to 4884f0c |
|
Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/972/) (Base Commit: 5d5983c) |
@jwforres @spadgett