k8s-stack: removed CRDs subchart as they are already included in operator #1459
Conversation
AndrewChubatiuk
requested review from
zekker6,
Haleygo and
f41gh7
as code owners
github-actions
bot
added
k8s-stack
AndrewChubatiuk
force-pushed
the
move-crds-to-separate-chart
branch
2 times, most recently
from
c228a23
to
8ca6ed8
Compare
AndrewChubatiuk
force-pushed
the
move-crds-to-separate-chart
branch
4 times, most recently
from
d469ce5
to
2d6db8f
Compare
github-actions
bot
added
agent
AndrewChubatiuk
force-pushed
the
move-crds-to-separate-chart
branch
from
99f8f05
to
8cd3fd9
Compare
| @@ -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.
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.
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.
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.
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.
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.
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.
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.
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.
don't like idea of having CRDs in both k8s-stack and operator charts, instead it's proposed to keep everything close to operator
AndrewChubatiuk
force-pushed
the
move-crds-to-separate-chart
branch
2 times, most recently
from
179d10e
to
623b529
Compare
AndrewChubatiuk
force-pushed
the
move-crds-to-separate-chart
branch
2 times, most recently
from
71a153c
to
7378008
Compare
AndrewChubatiuk
force-pushed
the
move-crds-to-separate-chart
branch
from
7378008
to
07d5330
Compare
AndrewChubatiuk
changed the title
moved CRDs to operators
crdsfolder to avoid importing them in both k8s-stack and operator charts