fix: crds getting removed on helm upgrades

[Abhinandan-Purkait]

Why do we need this PR?

It was seen that on upgrade to 1.5.x the CRDs get removed causing the removal of CRs. This would prevent it.
It also adds annotations to let helm keep the CRDs on uninstall, which is configurable.

[avishnu]

lgtm

[Ornias1993]

and this now leads to it bugging out when reinstalling in a cluster or installing on a cluster that previously used the crds folder.

Honestly: Does anyone here actually test these helm PR's, outside just a single initial install?
But even so, this behavior is already pretty clear by just reading the damn template... So what is going wrong here?

[Abhinandan-Purkait]

and this now leads to it bugging out when reinstalling in a cluster or installing on a cluster that previously used the crds folder.

Honestly: Does anyone here actually test these helm PR's, outside just a single initial install? But even so, this behavior is already pretty clear by just reading the damn template... So what is going wrong here?

For reinstalling or a fresh install with crds existing, users can use a simple set flag --set crds.csi.volumeSnapshots.enabled=false to them helm command.

I don't think that was too hard to figure out from the PR, anyway.

So I don't anything is going wrong here, we provided options, now users need to use what fits their use case.

If you still feel we don't know what we are doing please feel free to open an issue and you can contribute, we would be happy to review your PR.

[Ornias1993]

For reinstalling or a fresh install with crds existing, users can use a simple set flag --set crds.csi.volumeSnapshots.enabled=false to them helm command.

Obviously, but that was not the previous behavior.
So essentially you added quite big change in behavior without tagging it as break (which it, mater of factly is)

I don't think that was too hard to figure out from the PR, anyway.

No it isn't, but it's also not in your release notes as a clear breaking behavioral change.
It was also not hard to keep the "reinstall support" from previous version, within this change either.

So I don't anything is going wrong here, we provided options, now users need to use what fits their use case.

You significantly changed behavior, unneededly.

If you still feel we don't know what we are doing please feel free to open an issue and you can contribute, we would be happy to review your PR.

We likely are going to completely start rolling our own this summer, because the versioning and safety-of-updating of ZFS-LocalPV helm-charts is grossly unreliable, due to continuesly changing behavior.

[wolfgangwalther]

and this now leads to it bugging out when reinstalling in a cluster or installing on a cluster that previously used the crds folder.

"bugging out" as in "fails to install the chart, but doesn't destroy anything"?

Cool, I can very happily live with that - this is what I expected when I need to change some configuration.