feat(http-add-on): ClusterRole and ClusterRoleBinding use release name#844
feat(http-add-on): ClusterRole and ClusterRoleBinding use release name#844paihu wants to merge 2 commits into
Conversation
Signed-off-by: paihu <13479783+paihu@users.noreply.github.com>
|
Hi @paihu , |
ReviewContext: Issue #843 reports that installing the http-add-on chart multiple times (e.g., once per namespace with different What's good
Issues1. Incomplete -- aggregate ClusterRoles are missed
# rbac-aggregateclusterroles.yaml line 6
name: {{ .Chart.Name }}-edit
# rbac-aggregateclusterroles.yaml line 30
name: {{ .Chart.Name }}-viewThese would also collide in a multi-install scenario. They should be updated to 2. Breaking change -- orphaned resources on upgrade This is a breaking change for existing installations. When a user runs This should be documented in the chart's upgrade notes or changelog. The PR checklist item "README is updated with new configuration values" is unchecked -- at minimum, a note about the upgrade impact and manual cleanup steps would be valuable. 3. Minor: In the ClusterRoleBinding labels, some labels are changed and others aren't: labels:
keda.sh/addon: {{ .Chart.Name }} # stays .Chart.Name
app: {{ .Chart.Name }} # stays .Chart.Name
name: {{ .Release.Name }}-rolebinding # changed to .Release.NameThe Suggestions
|
Signed-off-by: paihu <13479783+paihu@users.noreply.github.com>
|
Thanks for the review. I had initially excluded aggregateclusterroles because it differs from the other ClusterRoles in that it grants permissions to users outside the chart and can be toggled on/off via For the ClusterRoles I changed in this PR, my assumption was that they're only used by roles within the chart, so the change wouldn't be breaking. However, if this change is also considered breaking, then I think it makes sense to update aggregateclusterroles along with the rest. |
http-add-on's ClusterRole and ClusterRoleBinding use
.Release.Nameinstead of.Chart.NameChecklist
Fixes #843