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

Change ClusterRoleBinding name to align with industry standard #1616

Merged

Conversation

ubergesundheit
Copy link
Contributor

@ubergesundheit ubergesundheit commented Feb 16, 2021

This PR changes the name of the ClusterRoleBinding. This appeared after running https://github.com/helm/chart-testing on the kedacore/charts repository.

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
  • A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified) Fix some linting issues charts#123
  • A PR is opened to update the documentation on (repo) (if applicable)
  • Changelog has been updated

@tomkerkhove
Copy link
Member

Same applies here: kedacore/charts#123 (comment)

@ubergesundheit
Copy link
Contributor Author

I'll leave this PR open until kedacore/charts#84 is merged if you don't mind

@ahmelsayed
Copy link
Contributor

@tomkerkhove is this good to merge?

@tomkerkhove
Copy link
Member

This relates to kedacore/charts#84, shall we go ahead with this @zroubalik?

This could be considered as breaking, though.

@zroubalik
Copy link
Member

@tomkerkhove I think that we can go with this one, but should mention somewhere, that there might be a leftover ClusterRoleBinding keda:system:auth-delegator when users will be updating from KEDA < 2.4

@tomkerkhove
Copy link
Member

Sounds good to me - @ubergesundheit would you mind rebasing main branch and adding an extra note to the changelog please?

@ubergesundheit
Copy link
Contributor Author

Sounds good to me - @ubergesundheit would you mind rebasing main branch and adding an extra note to the changelog please?

sure :)

@ubergesundheit ubergesundheit force-pushed the auth-delegator-clusterrolebinding-fix branch from 5a7bf24 to aecf96b Compare July 15, 2021 06:41
@ubergesundheit
Copy link
Contributor Author

The changelog entry now reads Fix keda-system-auth-delegatorClusterRoleBinding name ([#1616](https://github.com/kedacore/keda/pull/1616). Upgrading may leave a stray ClusterRoleBinding with the old namekeda:system:auth-delegator behind.

@zroubalik
Copy link
Member

The changelog entry now reads Fix keda-system-auth-delegatorClusterRoleBinding name ([#1616](https://github.com/kedacore/keda/pull/1616). Upgrading may leave a stray ClusterRoleBinding with the old namekeda:system:auth-delegator behind.

Would you mind moving it to ## Breaking Changes section? thx!

@ubergesundheit ubergesundheit force-pushed the auth-delegator-clusterrolebinding-fix branch from 288c815 to 92edf96 Compare July 15, 2021 07:28
@ubergesundheit
Copy link
Contributor Author

Are you going to squash this PR? Should I squash?

@tomkerkhove
Copy link
Member

No it's ok - I'll squash it!

@tomkerkhove tomkerkhove changed the title Correct ClusterRoleBinding name Change ClusterRoleBinding name to align with industry standard Jul 15, 2021
@tomkerkhove tomkerkhove merged commit 03d0f98 into kedacore:main Jul 15, 2021
@tomkerkhove
Copy link
Member

Are you willing to update our chart as well @ubergesundheit?

@ubergesundheit
Copy link
Contributor Author

@tomkerkhove you already have an open PR (created by you 😛 ) kedacore/charts#84

@tomkerkhove
Copy link
Member

😂

I'm a moron - Thank you!

@gldraphael
Copy link

gldraphael commented Sep 1, 2021

Hi, I'm currently on v2.3.2

Running a kubectl get clusterrolebindings returns the following:

NAME                                                   ROLE                                                               AGE
keda-operator                                          ClusterRole/keda-operator                                          50d
keda-operator-hpa-controller-external-metrics          ClusterRole/keda-operator-external-metrics-reader                  50d
keda-operator:system:auth-delegator                    ClusterRole/system:auth-delegator                                  50d

Is keda:system:auth-delegator a typo? Was it meant to be keda-operator:system:auth-delegator instead?


Edit:

It seems like the default is keda-operator:

https://github.com/kedacore/charts/blob/93e2fdc563974502f0d93707e4651c2f9bc63db3/keda/values.yaml#L23
https://github.com/kedacore/charts/blob/1e44265c3de39fb713d8ee278e86f17aa94c8380/keda/values.yaml#L15

nilayasiktoprak pushed a commit to nilayasiktoprak/keda that referenced this pull request Oct 23, 2021
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.

5 participants