Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

dumitraand
Copy link
Contributor

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.

@dumitraand dumitraand marked this pull request as ready for review August 14, 2024 06:52
@pmint93
Copy link
Owner

pmint93 commented Aug 15, 2024

@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
Copy link
Contributor Author

dumitraand commented Aug 22, 2024

@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
Copy link
Owner

pmint93 commented Aug 22, 2024

@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

@pmint93 pmint93 merged commit 1cd289b into pmint93:master Aug 22, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants