search

pmint93 / helm-charts

My helm charts
https://pmint93.github.io/helm-charts/
Apache License 2.0
73 stars 72 forks source link

[charts/metabase] (feat) Allow more DB Params from existing secrets #140

Closed dumitraand closed 1 month ago

dumitraand commented 1 month ago

Allows setting of DB_HOST, DB_PORT and DB_DBNAME from an existing secret.

I am not 100% sure this is the best way to do it, and I believe it might be better to do a refactor of the existingSecret like so:

existingSecret:
  name:
  usernameKey:
  passwordKey:
  hostKey:
  portKey:
  databaseNameKey:
  encryptionKeyKey:
  connetionUriKey:

But, this would've been a breaking change.

pmint93 commented 1 month ago

@dumitraand thanks for contributing

Agree with you about refactoring existingSecret will make it much cleaner. However, it's not necessary since it is a breaking change

About DB_HOST, the existingSecretConnectionURIKey can be used in your use case, isn't it ?

dumitraand commented 1 month ago

@pmint93 Thank you for your reply and it is my pleasure!

About DB_HOST, the existingSecretConnectionURIKey can be used in your use case, isn't it ?

That is true, but I just wanted to make all of the env variables available.

Likewise, I think we can argue that 

# existingSecretUsernameKey:
# existingSecretPasswordKey:

can also be fed into the existingSecretConnectionURIKey and we can challenge their presence as well. I wanted to give the option to either feed everything in a secret (existingSecretConnectionURIKey) or separately (with the other existingSecret*).

Regarding the breaking change, I'm more than happy to contribute and work on the breaking change in a separate PR, if you are ok with releasing a breaking change further down the line.

pmint93 commented 1 month ago

@dumitraand thank you for making it more clear about this PR

About the refactor, I'm glad you're willing to help but gotta say no, at least we're not getting any problem atm