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

[bitnami/external-dns:] Follow Helm rbac best practices #2589

Merged
merged 2 commits into from
May 15, 2020

Conversation

lazouz
Copy link
Contributor

@lazouz lazouz commented May 14, 2020

Description of the change
bitnami/external-dns : Follow Helm rbac best practices (https://helm.sh/docs/chart_best_practices/rbac/).
Enables the use of custom serviceAccount with custom clusteRole and clusterRoleBinding.

Benefits

Possible drawbacks

Changes in variables names : breaking changes

Applicable issues

Not known

Additional information

Checklist

  • Chart version bumped in Chart.yaml according to semver.
  • Variables are documented in the README.md
  • Title of the PR starts with chart name (e.g. [bitnami/chart])
  • If the chart contains a values-production.yaml apart from values.yaml, ensure that you implement the changes in both files

⚠️ Keep in mind that if you want to make changes to the kubeapps chart, please implement them in the kubeapps repository. This is only a synchronized mirror.

Copy link
Contributor

@juan131 juan131 left a comment

Choose a reason for hiding this comment

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

Hi @lazouz

Thanks so much for your PR!

As you mentioned, adopting these best practices implies including breaking changes. That means we will have to bump the major version of the chart, and provide users a workaround (under the "Upgrade" section of the README.md) so they can smoothly transition between major versions.

We are aware that sometimes our charts do not meet every Helm best practice, because some of the charts are prior to the existence of these best practices and adopting them has a price: backwards incompatibility.

That said, I am not saying we are rejecting your PR, but you need to play a little bit with the upgrade process and document it so we can accept it.

@lazouz lazouz force-pushed the lazouz/external-dns-rbac branch from 711586a to c7b5c8a Compare May 15, 2020 07:34
@lazouz lazouz force-pushed the lazouz/external-dns-rbac branch from c7b5c8a to e0a1f3a Compare May 15, 2020 07:37
@lazouz lazouz requested a review from juan131 May 15, 2020 07:38
Copy link
Contributor

@juan131 juan131 left a comment

Choose a reason for hiding this comment

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

Thanks so much for implementing the suggestions to document how to upgrade this chart.

Approving your PR!

@bitnami-bot
Copy link
Contributor

I have just updated the bitnami images with the latest known immutable tags:

  • "docker.io/bitnami/external-dns:0.7.1-debian-10-r56"

@juan131 juan131 merged commit 943a73d into bitnami:master May 15, 2020
@lazouz lazouz deleted the lazouz/external-dns-rbac branch May 15, 2020 10:14
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