-
Notifications
You must be signed in to change notification settings - Fork 291
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 best practices #240
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Welcome @snooyen! |
Hi @snooyen. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
CLA Signed |
/ok-to-test |
@snooyen Thank you for the PR. When upgrading from old chart to the new chart, the CRDs are being removed -
I believe this is because of the crds moved to |
/hold |
@aramase Which chart are you upgrading from that causes the CRDs to disappear? |
@snooyen Did you get a chance to try it out? Helm 3 uses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snooyen We can move the crds to a new dir, but to maintain backward compatibility and also to support users still using helm2, we need to add a crds.yaml
in the template
dir with
{{- range $path, $bytes := .Files.Glob "crds/*.yaml" }}
{{ $.Files.Get $path }}
---
{{- end }}
This will generate manifest as expected and ensure backward compatibility.
@aramase Sorry, I got pulled away to deal with other items. But I've just pushed the changes you requested. Tested locally, I was able to:
|
@snooyen Thank you for taking the time to work on this PR. As part of #258 we have created a new The |
/retitle helm: chart best practices |
@snooyen ping! Let us know if we can help with the PR. |
b836456
to
b5da249
Compare
b5da249
to
ff88924
Compare
keep crds in templates/ directory for helm2 compatibility
ff88924
to
c2762fc
Compare
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/hold cancel
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ritazh, snooyen The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
helm upgrade --install
due to changingrevision
label.Moves SecretProviderClass CRDs tocrds
directory. Prevents issues when installing on a cluster with pre-existing SecretProviderClass CRDS. (e.g.Error: UPGRADE FAILED: rendered manifests contain a new resource that already exists. Unable to continue with update: existing resource conflict: namespace: , name: secretproviderclasspodstatuses.secrets-store.csi.x-k8s.io, existing_kind: apiextensions.k8s.io/v1beta1, Kind=CustomResourceDefinition, new_kind: apiextensions.k8s.io/v1beta1, Kind=CustomResourceDefinition
)Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when the PR gets merged):Fixes #226, Fixes Azure/secrets-store-csi-driver-provider-azure#121
Special notes for your reviewer:
app
toapp.kubernetes.io/name
because spec.selector.matchLabels field is immutable. So theapp
label is maintained for chart backward compatibility with existing releases.