search

gissilabs / charts

Apache License 2.0
38 stars 22 forks source link

feat: add possibility to reference envs #56

Closed nico151999 closed 2 months ago

nico151999 commented 3 months ago

Added the possibility to reference ConfigMap and Secret keys as env variables. For implementing this the existing style of vaultwarden.extraEnv was used as inspiration so that the API design is more consistent.

Closes #55

sgissi commented 3 months ago

Thanks for the issue and the PR! I added some comments, let me know if it fits your use case.

nico151999 commented 3 months ago

Thanks for the response! Feeling a bit dumb but which comments do you refer to? I can't see any :sweat_smile:

sgissi commented 3 months ago

Thanks for the response! Feeling a bit dumb but which comments do you refer to? I can't see any 😅

Sorry about that, I forgot to publish the review 🤦

sgissi commented 3 months ago

@nico151999 Thanks for the update! I believe this is getting too complicated. Why not something like this:

extraEnv:
  ABC: "abc"
  FROM_SECRET:
    secretKeyRef:
      name: my-secret
      key: my-secret-key
  FROM_CONFIG:
    configMapKeyRef:
      name: my-config
      name: my-config-key

If the extraEnv entry has a string value (e.g. ABC above), it generates a static entry:

[...]
env:
- name: ABC
  value: "abc"

If type is a dictionary, we render a valueFrom:

[...]
env:
- name: FROM_SECRET
  valueFrom:
    secretKeyRef:
      name: my-secret
      key: my-secret-key

This way we don't need to introduce new variable, and no need to import the whole secret/configmap. It does need an update in documentation to explain that, and an example inside values.yaml.

nico151999 commented 3 months ago

That's fine by me. Will be home in like 30 minutes and adapt it accordingly. Looking forward to getting this implemented into my home setup. Hope you can publish a new release soon 😃

nico151999 commented 3 months ago

@sgissi check out the update I have just pushed. I think this suits your ask and my needs. Checked out the different use-cases I can think of in a local kind cluster. Seems to do its job 😃

nico151999 commented 3 months ago

@sgissi let me know if something is wrong about the PR and you would like the implementation to look different

nico151999 commented 2 months ago

@sgissi some time has passed. Has something gone wrong? If you don't want this merged please do me the favour of letting me know so I can simply build and use my own chart. Thanks.

sgissi commented 2 months ago

Hi @nico151999, thanks for the reminder! Nothing wrong with the patch, just didn't have time to merge and release yet.