Move crds to crds dir for helm3 and installCRDs flag for supporting helm3

[Evalle]

PR Summary

• Moves crds to crds dir for helm3 and installCRDs flag for supporting helm2
• Fixes Move crds to crds dir for helm3 #286

Special notes for the reviewer:
@aramase have moved crds to crds dir in charts/secrets-store-csi-driver should I do the same ing manifest-staging ?

[k8s-ci-robot]

Welcome @Evalle!

It looks like this is your first PR to kubernetes-sigs/secrets-store-csi-driver 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/secrets-store-csi-driver has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @Evalle. 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.

[aramase]

@Evalle Thank you for the PR. All the changes are to be made in manifest_staging/charts/secrets-store-csi-driver only. That contains the latest charts and yamls that will be moved to charts at the time of release so we can ensure all chart and yaml changes are tagged with a release. So could you update the PR to make the changes in manifest_changing instead?

Also, as I mentioned in the issue, we would need to add template/crds.yaml to make the charts work with helm2 as well. That blob would need to be under a conditional installCRDs, which users can set true for helm2 crd install. helm2 isn't aware of crds dir, so it would skip crd installation otherwise. Please feel free to ping on the issue or on slack if you have any questions.

[Evalle]

@aramase thanks for the feedback. I've addressed your comment, could you please re-review? Thanks!

[aramase]

nit: new line

[aramase]

@Evalle Could you also add installCRDs to values file and set it to false by default. This flag needs to be added to the configuration table and a note in the readme on --set installCRDs=true if using helm2 to install the chart.

[Evalle]

@aramase everything is done except for

and a note in the readme on --set installCRDs=true if using helm2 to install the chart.

In which README.mdI should add it? There are three of them, two of which has a prerequisite Helm 3.0

Thanks

[aramase]

In which README.mdI should add it? There are three of them, two of which has a prerequisite Helm 3.0

@Evalle It would be in manifest_staging/charts/secrets-store-csi-driver/README.md. We need instructions for helm2 and how installCRDs=true needs to be set only for helm2.

[Evalle]

@aramase I've addressed all your comments, could you please re-review? Thx!

[ritazh]

@Evalle can you pls also update https://github.com/kubernetes-sigs/secrets-store-csi-driver/blob/master/Makefile#L162 to cp the files to the new crds folder instead?

[ritazh]

/ok-to-test

[aramase]

@Evalle Are you interested in picking up this PR again? We're planning to include the move to helm3 in the next release v0.1.0. The helm charts will only be supported with helm3, so we no longer need the installCRDs flag and crds.yaml in the templates folder.

Essentially the required changes are:

1. Move the 2 CRDs to crds directory
2. Update the apiVersion from v1 to v2 in here.

Let us know if you would like to rebase and update this PR.

[Evalle]

Progress:

• remove installCRDs flag
• remove crds.yaml in the templates folder.
• Move the 2 CRDs to crds directory - @aramase should I create the crds folder in the root? CRDs are in secrets-store-csi-driver/manifest_staging/charts/secrets-store-csi-driver/crds/ now
• Update the apiVersion from v1 to v2

p.s. I'm going to rebase my commits after I'm done.

[aramase]

Move the 2 CRDs to crds directory - @aramase should I create the crds folder in the root? CRDs are in secrets-store-csi-driver/manifest_staging/charts/secrets-store-csi-driver/crds/ now

Looks like you've the crds directory setup correctly in the root.

[aramase]

/label tide/merge-method-squash

[aramase]

/lgtm

[aramase]

/cc @tam7t

[tam7t]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Evalle, tam7t

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 [tam7t]

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

[aramase]

/hold cancel

[ritazh]

Thank you for completing this PR!

Has there been more discussion/thoughts around this concern? Perhaps a pre-upgrade hook? IMO, we should figure this out before cutting v0.1.0.

Helm will install the crds in the crds/ directory if the crds are not already installed. If the crds are installed, it will not validate that the version installed matches the served/stored versions specified in the manifests. On upgrade it doesn't touch the crds/ directory at all.