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

fix: honor .global.postgresql.auth values #154

Merged
merged 1 commit into from
Dec 1, 2023

Conversation

tumido
Copy link
Member

@tumido tumido commented Nov 24, 2023

Description of the change

Our dependency - bitnami/postgresql chart has an option to configure auth via global scope variables. We do not consider that option when we infer secret name and key for the Backstage deployment volume mounts.

This PR adds support for:

  1. .Values.global.postgresql.auth.existingSecret which overrides .Values.postgresql.auth.existingSecret
  2. .Values.global.postgresql.auth.secretKeys.userPasswordKey which overrides .Values.postgresql.auth.secretKeys.userPasswordKey and (either of these) gets applied only if
    .Values.global.postgresql.auth.existingSecret or .Values.postgresql.auth.existingSecret is defined

I'm using () chaining Helm ?. pipeline notation/workaround so I can avoid declaring those variables as '' in values.yaml

Existing or Associated Issue(s)

Additional Information

Checklist

  • Chart version bumped in Chart.yaml according to semver.
  • Variables are documented in the values.yaml and added to the README.md. The helm-docs utility can be used to generate the necessary content. Use helm-docs --dry-run to preview the content.
  • JSON Schema generated.
  • List tests pass for Chart using the Chart Testing tool and the ct lint command.

@tumido
Copy link
Member Author

tumido commented Nov 27, 2023

cc @vinzscam @sabre1041 @ChrisJBurns

Can you please take a look and let me know what you think? 🙂

Copy link
Contributor

@sabre1041 sabre1041 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Great work

Approving but added a comment for your consideration

charts/backstage/templates/_helpers.tpl Show resolved Hide resolved
Signed-off-by: Tomas Coufal <tcoufal@redhat.com>
Copy link
Contributor

@sabre1041 sabre1041 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ChrisJBurns
Copy link
Contributor

ChrisJBurns commented Dec 1, 2023

I get scared when I see postgres auth secret values getting changed 😆, mainly because of past stress of trying to satisfy all of the scenarios that tend to cause regression to other values if not done right. Do we have good tests to ensure that when certain values are set, the correct Deployment yaml is generated? As theres a few conditional branches in that backstage.postgresql.databaseSecretKey function. If not, would be good to get these in another time if possible. Will allow us to change any code in this area with increased confidence in future.

I will add though, changes lgtm 👍

@tumido
Copy link
Member Author

tumido commented Dec 1, 2023

I don't have any tests at this time @ChrisJBurns . I've logged an issue to address this in future. I fully agree. 🙂

@tumido tumido merged commit 1b41758 into backstage:main Dec 1, 2023
3 checks passed
@tumido tumido deleted the honor-postgresq-global branch December 1, 2023 15:43
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.

3 participants