timescale / helm-charts

Configuration and Documentation to run TimescaleDB in your Kubernetes cluster
Apache License 2.0
264 stars 223 forks source link

*SecretName: only append suffix when no values are provided #359

Closed con5cience closed 2 years ago

con5cience commented 2 years ago

In the Docs: https://github.com/timescale/timescaledb-kubernetes/blob/master/charts/timescaledb-single/admin-guide.md#alternative-way-of-handling-secrets

Alternativelly [sic] the chart allows referencing existing Secret objects via secret.credentialsSecretName

In the Code: https://github.com/timescale/timescaledb-kubernetes/blob/master/charts/timescaledb-single/templates/secret-patroni.yaml#L13

printf "%s-credentials"

😡

In this PR: Append suffixes to the names of secrets objects only when input values are not provided by the user.

CLAassistant commented 2 years ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

:white_check_mark: paulfantom
:white_check_mark: con5cience
:x: William Stiern


William Stiern seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

con5cience commented 2 years ago

We are relying on the existence of this suffix in tobs. It might be better to document the requirement of this suffix instead of changing the logic.

The same goes for -certificate and -pgbackrest. We shouldn't change/document this only for one secret.

Change the reference in future versions of tobs then? That's whole point, to use the raw value that was passed in and not rely on mutating it or requiring it to be formatted in a specific fashion in order for it to work properly.

I've pushed another commit to do the same for -certificate and -pgbackrest.

paulfantom commented 2 years ago

LGTM, you just need to sign the CLA so we can merge it.

con5cience commented 2 years ago

LGTM, you just need to sign the CLA so we can merge it.

I did. I also have many PGP/SSH keys bound to this account, not sure how the bot is determining identity.

paulfantom commented 2 years ago

Fixed in #373