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.

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

deploy: Use aggregated ClusterRoles #322

Merged
merged 1 commit into from
Apr 26, 2019
Merged

Conversation

kshlm
Copy link
Contributor

@kshlm kshlm commented Apr 16, 2019

The kubernetes manifests and Helm templates have been updated to use
aggregated ClusterRoles. The same change has been done in Rook as well.

Refer rook/rook#2634 and rook/rook#2975

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Apr 16, 2019

@kshlm CI failure

rbac.ceph.rook.io/aggregate-to-{{ include "ceph-csi-cephfs.nodeplugin.fullname" . }}: "true"
rules: []
---
kind: ClusterRole
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kfox1111 PTAL

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Apr 16, 2019

@kshlm do we need to use rook labels and selectors here?

@kshlm kshlm force-pushed the aggregated-roles branch from 0f92e31 to 3b6d32a Compare April 16, 2019 08:11
@kshlm
Copy link
Contributor Author

kshlm commented Apr 16, 2019

@kshlm do we need to use rook labels and selectors here?

Not really. I just used the same labels I used in the rook PR.

@kfox1111
Copy link
Contributor

oh, this is very interesting... You taught me something new about rbac. Thanks. :)

I agree, it feels a little weird for rook to be used as a label here. Please update to be something in the ceph namespace.

For the helm chart, please break the two different resources into two different files. The helm convention is to have one object per file. say, nodeplugin-clusterrole.yaml and nodeplugin-rules-clusterrole.yaml

Overall, looks to be a good change. Thanks.

@kfox1111
Copy link
Contributor

Oh. and please bump the helm chart versions.

@kshlm kshlm force-pushed the aggregated-roles branch from 3b6d32a to c459ae9 Compare April 17, 2019 05:37
The kubernetes manifests and Helm templates have been updated to use
aggregated ClusterRoles. The same change has been done in Rook as well.

Refer rook/rook#2634 and rook/rook#2975

Signed-off-by: Kaushal M <kshlmster@gmail.com>
@kshlm kshlm force-pushed the aggregated-roles branch from c459ae9 to 63d00af Compare April 17, 2019 05:45
Copy link
Contributor

@ShyamsundarR ShyamsundarR left a comment

Choose a reason for hiding this comment

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

Thanks, looks good.

@humblec
Copy link
Collaborator

humblec commented Apr 24, 2019

@kfox1111 any final comments from your side ? This looks good to me..

@travisn travisn mentioned this pull request Apr 24, 2019
5 tasks
@rootfs rootfs merged commit bc2624f into ceph:master Apr 26, 2019
wilmardo pushed a commit to wilmardo/ceph-csi that referenced this pull request Jul 29, 2019
deploy: Use aggregated ClusterRoles
Nikhil-Ladha pushed a commit to Nikhil-Ladha/ceph-csi that referenced this pull request Jul 26, 2024
Syncing latest changes from upstream devel for ceph-csi
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants