Skip to content
This repository has been archived by the owner on Feb 2, 2024. It is now read-only.

fix: update default values to account for postgresql existingSecret #148

Merged
merged 1 commit into from
Nov 28, 2023

Conversation

PatAKnight
Copy link
Member

Description of the change

Adds two new functions that will be used to set the values at upstream.backstage.extraEnvVars.valueFrom.secretKeyRef.name and upstream.postgresql.primary.extraEnvVars.valueFrom.secretKeyRef.name. These functions will check if upstream.postgresql.auth.existingSecret is set and set the above values to this value. Otherwise it will set them to release-name-postgresql

Existing or Associated Issue(s)

Additional Information

I am setting this as a draft so that I can get it reviewed to ensure that this is the route that we wish to go.

Checklist

  • Chart version bumped in Chart.yaml according to semver.
  • Variables are documented in the values.yaml and added to the README.md. The pre-commit utility can be used to generate the necessary content. Use pre-commit run -a to apply changes.
  • JSON Schema template updated and re-generated the raw schema via pre-commit hook.
  • List tests pass for Chart using the Chart Testing tool and the ct lint command.

Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

Just a suggestion for enhancement 🙂

charts/backstage/templates/_helpers.tpl Outdated Show resolved Hide resolved
@PatAKnight PatAKnight force-pushed the existing-secret branch 2 times, most recently from 8886ea8 to 14d4a11 Compare November 16, 2023 13:08
@PatAKnight PatAKnight marked this pull request as ready for review November 21, 2023 13:30
@PatAKnight PatAKnight requested a review from a team November 21, 2023 13:30
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

Just a single comment, otherwise LGTM 🙂

charts/backstage/values.yaml Outdated Show resolved Hide resolved
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

I've left few suggestions inline... I think we should be consistent with upstream in value priority.

In addition to that, I've fixed the thing you've mentioned as broken in the upstream Backstage chart:

backstage/charts#154

charts/backstage/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/backstage/values.yaml Outdated Show resolved Hide resolved
charts/backstage/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/backstage/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/backstage/templates/_helpers.tpl Outdated Show resolved Hide resolved
@PatAKnight PatAKnight force-pushed the existing-secret branch 2 times, most recently from 67f1f1d to 40ece3b Compare November 27, 2023 13:53
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

I have 1 nit remaining. In addition to that, can you please rebase and bump the version once more? 🙂

charts/backstage/templates/_helpers.tpl Show resolved Hide resolved
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@tumido tumido merged commit b3bedfb into janus-idp:main Nov 28, 2023
2 of 3 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting existingSecret for PostgreSQL is not respected by our default values
2 participants