-
Notifications
You must be signed in to change notification settings - Fork 328
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
k8s-stack: removed CRDs subchart as they are already included in operator #1459
base: master
Are you sure you want to change the base?
Conversation
c228a23
to
8ca6ed8
Compare
d469ce5
to
2d6db8f
Compare
99f8f05
to
8cd3fd9
Compare
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!
See one comment.
@@ -2,7 +2,8 @@ | |||
|
|||
## Next release | |||
|
|||
- TODO | |||
- Moved crds to a shared chart and import them as a dependency | |||
- replaced `crd.*` properties to `crds.*` as they now belong to a subchart |
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.
Please mention this as a upgrade note too, as it requires user to change their values if they used those properties before.
@@ -1,18 +0,0 @@ | |||
{{- /* do not update crds here, please update in /victoria-metrics-operator/crd.yaml */ -}} |
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.
It's breaking change that will have strongly negative impact on chart users.
Helm cannot perform CRD updates according to the documentation:
https://helm.sh/docs/chart_best_practices/custom_resource_definitions/#some-caveats-and-explanations
And it was the main point of keeping CRDs at templates.
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.
I had the same wrong impression( but resources with annotation helm.sh/resource-policy: keep
won't be upgraded either during helm upgrade
. So this pull request doesn't bring breaking change.
The annotation helm.sh/resource-policy: keep instructs Helm to skip deleting this resource when a helm operation (such as helm uninstall, helm upgrade or helm rollback) would result in its deletion.
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.
We might need to add some instructions to explain this and tell users how to upgrade crds for operator and k8s-stack charts, wdyt @AndrewChubatiuk
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.
i've just checked helm code, this annotation prevents removal during upgrade, but seems like resource update itself should happen
https://github.com/helm/helm/blob/49819b4ef782e80b0c7f78c30bd76b51ebb56dc8/pkg/kube/client.go#L214
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.
Sounds like a bug at helm side. Because documentation wording mentions only deleting
resource case. It should be upgrade on regular basis.
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.
Helm cannot perform CRD updates according to the documentation
but crds are in templates in operator chart only, k8s-stack has them in crds
folder of a subchart
I can update PR and leave it for user to decide if he wants crd as templates or as immutable object
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.
Because documentation wording mentions only deleting resource case. It should be upgrade on regular basis.
Sorry, I did the wrong test, that's true and we shouldn't change it for operator chart.
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.
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.
don't like idea of having CRDs in both k8s-stack and operator charts, instead it's proposed to keep everything close to operator
179d10e
to
623b529
Compare
71a153c
to
7378008
Compare
7378008
to
07d5330
Compare
moved CRDs to operators
crds
folder to avoid importing them in both k8s-stack and operator charts