search

danfromtitan / envars-from-node-labels

Export container environment variables from K8s node labels
Apache License 2.0
24 stars 4 forks source link

support multiple namespaces - breaking change #20

Closed balonik closed 1 year ago

balonik commented 1 year ago

This is a BREAKING CHANGE, because the type of namespaceSelector is changed from string to array

Rewrite the MutatingWebhookConfiguration to allow adding multiple namespaces. Fixes https://github.com/danfromtitan/envars-from-node-labels/issues/16

danfromtitan commented 1 year ago

Do you mind increasing the helm chart version to reflect the breaking change (i.e. make it 0.2.0)

balonik commented 1 year ago

Actually, almost immediately after I implemented this change it got to me that I could just easily put a label with same value on all namespaces and the result will be the same. This change won't be needed really. Optionally, I would just change the values.yaml a bit to make the names more straight forward and adjusted the mutatingwebhookconfiguration accordingly. I'd change the namespaceSelector and the recently added namespaceSelectorLabel into:

namespaceSelector:
  labelKey: name
  labelValue: sample

This change would make sense if someone needs to use existing label that has different value on namespaces, though. What do you think?

danfromtitan commented 1 year ago

This change would make sense if someone needs to use existing label that has different value on namespaces, though. What do you think?

That would obscure the configuration, I like using the list of namespaces because it's more explicit.

Please increase the chart version and I'll merge.

balonik commented 1 year ago

Done.

danfromtitan commented 1 year ago

Tests passed, thank you for your contribution.