[helm-chart] csi-snapshotter in ebs-csi-controller now checks for enableVolumeSnapshot before including it in containers

[missingcharacter]

Is this a bug fix or adding new feature?
bug fix

What is this PR about? / Why do we need it?
Fixes #942

What testing is done?
locally, yes

[k8s-ci-robot]

Hi @missingcharacter. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

[wongma7]

This doesn't fully fix the issue because we might have users who already have the controller and CRDs installed, so they set enableVolumeSnapshot false. In that case they DO want the sidecar still running.

TBH I'm leaning heavily toward not installing the snapshot controller and CRDS at all : ( #635 (comment), let me know your thoughts.

[wongma7]

/ok-to-test

[missingcharacter]

@wongma7

I intend to remove the CRDs and controller and simply list it as a prerequisite to the chart

This makes total sense, I think it is best to NOT install the controller nor CRDs and just list them as pre-requisites

[wongma7]

Yeah my full reasoning is in the comment, @krmichel if we want to stop installing the CRDs and snapshot controller, and instead point users to docs to how to install it (leveraging the help text that prints right after an install) do we need to bump the helm chart major version?

[missingcharacter]

@wongma7

do we need to bump the helm chart major version?

Yes, please 🙇🏽‍♂️ (I know you are not asking me)

[wongma7]

OK. I'm actually prepping helm chart 1.2.4 with your previous PR right now. So that will leave the 1.x.x series of charts in an okay-ish state (if you set enableVolumeSnapshot true it will install everything you need, if it's false then you might run into #942)

So, after 1.2.4 is released we can take some time to make any other breaking changes we want.

(EFS repo made a 2.0.0 release recently as well , so I am not worried at all about making a new major version too fast or whatever.)

[missingcharacter]

Actually, I think I can add a capabilities check to install the sidecar if enableVolumeSnapshot is true or the snapshot CRDs are present, it is not perfect but it would leave 1.2.4 in a better state

[krmichel]

Yes, please 🙇🏽‍♂️ (I know you are not asking me)

I fully agree. I already have the second half of the major chart update that needs to do things like getting rid of the old not well-organized values and bump the helm version requirement so I will try to get those done and in PR so we can roll them into 2.0 as well.

[missingcharacter]

Ok, this PR should leave 1.2.4 in better shape to then remove CRDs and snapshot controller off it

[wongma7]

/lgtm

I'll rebase on this / include it in #959

[wongma7]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: missingcharacter, wongma7

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:

• OWNERS [wongma7]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment