search

clastix / kamaji

Kamaji is the Hosted Control Plane Manager for Kubernetes.
https://kamaji.clastix.io
Apache License 2.0
992 stars 90 forks source link

Ability to set an existing Datastore Custom Resource when deploying Kamaji #383

Closed EBMBA closed 9 months ago

EBMBA commented 10 months ago

Hi,

It could be a useful feature to be able to set an existing Datastore Custom Resource when deploying Kamaji.

Context : If I deploy an etcd cluster using your Kamaji-ETCD Helm chart an I set datastore.enabled value to true :

datastore:
  # -- Create a datastore custom resource for Kamaji
  enabled: true

I will have a new datastore custom resource deployed, named kamaji-etcd. And then if I want to deploy Kamaji using your Helm chart. I would like to use my existing datastore cr kamaji-etcd. Instead of configuring a new one or using the default one.

But the default datastore cr can't be skipped as the default etcd.

prometherion commented 10 months ago

I think we're missing the required knob for this file: https://github.com/clastix/kamaji/blob/95de31d6970a98e46bdee4ab7638cbb3311b5127/charts/kamaji/templates/datastore.yaml#L1-L26

We could easily implement something like this: https://github.com/clastix/kamaji/blob/95de31d6970a98e46bdee4ab7638cbb3311b5127/charts/kamaji/templates/etcd_cm.yaml#L1

And if a user would like to use an externally managed Datastore as default, take advantage of the datastore.nameOverride variable. https://github.com/clastix/kamaji/blob/95de31d6970a98e46bdee4ab7638cbb3311b5127/charts/kamaji/values.yaml#L159-L161

Would you like to contribute to this, @EBMBA ?

EBMBA commented 10 months ago

Hi @prometherion

Yes I would like to contribute to this improvement !

EBMBA commented 10 months ago

I added an "enabled" parameter in the chart values : https://github.com/EBMBA/kamaji/blob/b0856eeea57716f361c1e36c34a9ea6453f0d5d8/charts/kamaji/values.yaml#L161

The "enabled" parameter will determine whether the datastore will be created or not : https://github.com/EBMBA/kamaji/blob/b0856eeea57716f361c1e36c34a9ea6453f0d5d8/charts/kamaji/templates/datastore.yaml#L1

If datastore.enabled is set to false, a datastore.nameOverride value will be required : https://github.com/EBMBA/kamaji/blob/b0856eeea57716f361c1e36c34a9ea6453f0d5d8/charts/kamaji/templates/_helpers_datastore.tpl#L8

That's all changes that have been done.

prometherion commented 10 months ago

That would be better having a review in your PR rather than the issue, but overall the proposed changes are good!

Open the PR so we can get it merged!