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

Helm Chart update: make executor ClusterRole name unique #925

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bgounon
Copy link

@bgounon bgounon commented Feb 19, 2024

Summary

Executor ClusterRole name should be dynamic instead of hard-coded. Without it, you cannot have several namespaced releases of k8up, as it triggers a collision on the executor ClusterRole name and fails.

Checklist

For Helm Chart changes

  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog
  • PR contains the label area:chart
  • PR contains the chart label, e.g. chart:k8up
  • Variables are documented in the values.yaml using the format required by Helm-Docs.
  • Chart Version bumped if immediate release after merging is planned
  • I have run make chart-docs
  • Link this PR to related code release or other issues.

@bgounon bgounon requested a review from a team as a code owner February 19, 2024 17:33
@bgounon bgounon requested review from TheBigLee and wejdross and removed request for a team February 19, 2024 17:33
@bgounon bgounon marked this pull request as draft February 19, 2024 17:33
@bgounon bgounon changed the title Helm Chart update: make executor ClusterRole unique Helm Chart update: make executor ClusterRole name unique Feb 19, 2024
@bgounon bgounon marked this pull request as ready for review February 19, 2024 17:36
Signed-off-by: Bastien Gounon <45844674+bgounon@users.noreply.github.com>
@Kidswiss
Copy link
Contributor

Kidswiss commented Mar 6, 2024

Hi @bgounon

Thank you for the PR!

I like the idea, but just making the name dynamically based on the fullname could also break single installations, if a non-default name is specified.

Instead, you could add a new value under rbac like disableClusterRole. Then the first k8up install can install the clusterrole and all subsequent ones don't.

Another approach could be to disable the rbac completely and deploy the ClusterRoles separately, via the value rbac.create.

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.

2 participants