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

feat: role and rolebinding template change to allow name override. #334

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

Greyeye
Copy link
Collaborator

@Greyeye Greyeye commented Dec 18, 2024

  1. change to use kubechecks.fullname for the role name and role binding name.

@Greyeye Greyeye changed the title role and rolebinding change. feat: role and rolebinding template change to allow name override. Dec 18, 2024
1. change to use kubechecks.fullname for the role name and role binding name.
@zapier-sre-bot
Copy link
Collaborator

Mergecat's Review

Click to read mergecats review!

😼 Mergecat review of charts/kubechecks/Chart.yaml

@@ -1,7 +1,7 @@
 apiVersion: v2
 name: kubechecks
 description: A Helm chart for kubechecks
-version: 0.5.0
+version: 0.5.1
 type: application
 maintainers:
   - name: zapier

Feedback & Suggestions: The version bump from 0.5.0 to 0.5.1 suggests a patch update, which typically indicates bug fixes or minor improvements. Ensure that the changes in the codebase reflect this type of update. If there are significant changes, consider updating the version number accordingly. Additionally, it would be helpful to update the changelog or release notes to document what changes have been made in this version. 📈


😼 Mergecat review of charts/kubechecks/templates/role.yaml

 apiVersion: rbac.authorization.k8s.io/v1
 kind: Role
 metadata:
-  name: kubechecks
+  name: {{ include "kubechecks.fullname" . }}
   namespace: {{ .Values.argocd.namespace }}
 rules:
   - apiGroups:

Feedback & Suggestions:

  1. Template Consistency: The change to use {{ include "kubechecks.fullname" . }} for the role name is a good practice for ensuring consistency and uniqueness across different deployments. However, ensure that the kubechecks.fullname template is defined correctly in your Helm chart to avoid rendering issues. 🛠️

  2. Testing: After making this change, test the Helm chart to ensure that the role name is rendered correctly and that it does not conflict with existing resources. This is especially important if the fullname template includes dynamic elements like release names or namespaces. 🔍

  3. Documentation: Update any relevant documentation to reflect this change, especially if users need to be aware of the new naming convention for roles. 📚


😼 Mergecat review of charts/kubechecks/templates/rolebinding.yaml

 apiVersion: rbac.authorization.k8s.io/v1
 kind: RoleBinding
 metadata:
-  name: kubechecks
+  name: {{ include "kubechecks.fullname" . }}
   namespace: {{ .Values.argocd.namespace }}
 roleRef:
   kind: Role

Feedback & Suggestions:

  1. Consistency in Naming: The change from a static name to using {{ include "kubechecks.fullname" . }} for the name field can be beneficial for ensuring consistency across resources. However, ensure that kubechecks.fullname is defined correctly in your Helm chart templates to avoid any potential issues with resource naming.

  2. Template Function Verification: Double-check that the kubechecks.fullname template function is returning the expected value. This is crucial to avoid any unintended naming that could lead to conflicts or misconfigurations.

  3. Documentation Update: If this change affects how users interact with the chart (e.g., if they need to be aware of the new naming convention), consider updating any relevant documentation to reflect this change.

  4. Testing: Ensure that this change is tested in a staging environment to verify that the RoleBinding is created with the correct name and that it functions as expected.



Dependency Review

Click to read mergecats review!

No suggestions found

Copy link

github-actions bot commented Dec 18, 2024

Temporary image deleted.

@Greyeye Greyeye merged commit 0efc311 into main Dec 18, 2024
4 checks passed
@Greyeye Greyeye deleted the role-name-setter branch December 18, 2024 03:24
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.

3 participants