-
Notifications
You must be signed in to change notification settings - Fork 116
https://issues.redhat.com/browse/ACM-23588 validate spec for cluster permissions #8350
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
Conversation
release_notes/acm_new.adoc
Outdated
|
|
||
|
|
||
|
|
||
| - You can now enable the `validate` specification to check the accuracy of your `Role` and `ClusterRole` resources. For more information, see link:../secure_clusters/cluster_val.adoc#enable-cluster-val[Enabling validation for cluster permissions]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the word accuracy doesn't make sense here. The validate feature just checks if the Role or ClusterRole exists. So can we change accuracy to existence? Thanks!
secure_clusters/cluster_val.adoc
Outdated
| [#enable-cluster-val] | ||
| = Enabling validation for cluster permissions | ||
|
|
||
| Enable the `validate` specification within your `ClusterPermission` resources to check the accuracy of your `Role` and `ClusterRole` resources. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the comment above. Change accuracy to existence.
jc-berger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
secure_clusters/cluster_val.adoc
Outdated
| @@ -0,0 +1,52 @@ | |||
| [#enable-cluster-val] | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should change this file name to cluster_permission_validate.adoc --- this can get confused with cluster lifecycle, meaning "cluster validation" and the val can get confused with "value" -- it's one of those cases where we need the three word file name.
|
/lgtm |
|
@dockerymick It looks good to me. FYI @fxiang1 is the dev owner of this feature. I noticed his comments have been addressed. It would be better to let Feng double review it. |
|
@xiangjingli sure thing! Thank you! |
|
/lgtm Thanks @dockerymick ! |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dockerymick, fxiang1, jc-berger, swopebe, xiangjingli The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Thanks everyone! Merging |
No description provided.