openebs / lvm-localpv

Dynamically provision Stateful Persistent Node-Local Volumes & Filesystems for Kubernetes that is integrated with a backend LVM2 data storage stack.
Apache License 2.0
235 stars 92 forks source link

fix(helm): remove CRDs already part of kubernetes-csi/external snapshotter #226

Closed rhr323 closed 1 year ago

rhr323 commented 1 year ago

Why is this PR required? What issue does it fix?:

The helm chart includes CRDs that are part of kubernetes-csi/external-snapshotter. This causes some confusion when the CRDs are installed from two different sources.

What this PR does?:

This PR removes the CRDs that are already part of other kubernetes components.

Does this PR require any upgrade changes?:

Helm does not remove CRDs, so no longer having the CRDs in the helm chart should not cause any issues.

If the changes in this PR are manually verified, list down the scenarios covered::

Any additional information for your reviewer? :

This is a proposal, and would be happy to receive feedback, and/or understand the original reason why the CRDs have been duplicated to the helm chart of openebs/lvm-localpv.

Checklist:

PLEASE REMOVE BELOW INFORMATION BEFORE SUBMITTING

The PR title message must follow convention: <type>(<scope>): <subject>.

Where:

rhr323 commented 1 year ago

Hey @niladrih , thanks for the response!

Maybe an other solution could be to wrap the CRDs in question, for example:

{{- if .Values.k8sStorageCrds.create }}
---
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  name: volumesnapshotclasses.snapshot.storage.k8s.io

....

{{- end }}

This way, users of openebs could decide whether they would like to install those CRDs or not... What do you think?

niladrih commented 1 year ago

Sounds good to me @rhr323 . Let's leave installation on by default though...

niladrih commented 1 year ago

@rhr323 this change has been included in PR https://github.com/openebs/lvm-localpv/pull/243, closing this one.