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

Change postgresql.useSubChart to .enabled #341

Merged
merged 6 commits into from
Jun 5, 2024

Conversation

mitchnielsen
Copy link
Contributor

@mitchnielsen mitchnielsen commented May 31, 2024

Summary

Changes postgresql.useSubChart to postgresql.enabled, following the common convention mentioned in https://helm.sh/docs/topics/charts/https://helm.sh/docs/topics/charts/.

This should hopefully make it clearer what that key does - and this key already existed, so by removing useSubChart, there's only one key to manipulate.

Other commits in this PR have commit messages explaining what and why.

Closes #336

Testing

Internal PostgreSQL enabled (default)

(Default values used)

$ helm template test . -f test.values.yaml --show-only charts/postgresql/templates/primary/statefulset.yaml | yq '.metadata.name'
test-postgresql

Internal PostgreSQL disabled

postgresql:
  enabled: false
$ helm template test . -f test.values.yaml --show-only charts/postgresql/templates/primary/statefulset.yaml | yq '.metadata.name'
Error: could not find template charts/postgresql/templates/primary/statefulset.yaml in chart
null

Internal PostgreSQL + default Secret

(Default values used)

$ helm template test . -f test.values.yaml --show-only templates/deployment.yaml| yq '.spec.template.spec.containers[0].env[] | select(.name == "PREFECT_API_DATABASE_CONNECTION_URL") | .valueFrom.secretKeyRef.name'
prefect-server-postgresql-connection

Internal PostgreSQL + existing Secret

postgresql:
  enabled: true
  auth:
    existingSecret: my-external-pg-secret
$ helm template test . -f test.values.yaml --show-only templates/deployment.yaml| yq '.spec.template.spec.containers[0].env[] | select(.name == "PREFECT_API_DATABASE_CONNECTION_URL") | .valueFrom.secretKeyRef.name'
my-external-pg-secret

External PostgreSQL + existing Secret

postgresql:
  enabled: false
  auth:
    existingSecret: my-external-pg-secret
$ helm template test . -f test.values.yaml --show-only templates/deployment.yaml| yq '.spec.template.spec.containers[0].env[] | select(.name == "PREFECT_API_DATABASE_CONNECTION_URL") | .valueFrom.secretKeyRef.name'
my-external-pg-secret

Changes 'useSubChart' to 'enabled'https://helm.sh/docs/topics/charts/
to follow common convention as mentioned in
https://helm.sh/docs/topics/charts/.

This should make it more clear what this key does.

Closes #336
Removes the postgresql.enabled check on the PostgreSQL Secret injection
in the Deployment.

Turning off the installation of the chart shouldn't prevent a secret
from being injected, since a user can provide an existing Secret name.
@mitchnielsen
Copy link
Contributor Author

mitchnielsen commented May 31, 2024

Jotting down action items for myself:

Caught up with @jamiezieziula on this. The postgresql.auth key in values.yaml is Prefect-specific, so we need to allow auth settings to be set outside of the scope of postgresql when the built-in PG chart isn't used (postgresql.enabled=false).

We'll also want to support passing no auth and no database for those who want to test with the SQLite DB.


EDIT: following up here, you can still pass values to the postgresql.auth key even when postgresql.enabled=false and we'll still consider the values there. This is because we no longer check if postgresql.enabled=true when injecting the Secret into the Deployment (31c8577). The name of that Secret is calculated by a helper which will consider auth.existingSecret if specified.

Documents how to disable the bundled PostgreSQL instance and provide the
auth Secret.

This Secret will be injected into the Prefect Server Deployment.
Looking at the Secret template, we no longer need a `password` key in
the existingSecret object.
@mitchnielsen mitchnielsen self-assigned this Jun 4, 2024
@mitchnielsen mitchnielsen added the enhancement An improvement of an existing feature label Jun 4, 2024
@mitchnielsen mitchnielsen marked this pull request as ready for review June 4, 2024 19:51
@mitchnielsen mitchnielsen requested a review from a team as a code owner June 4, 2024 19:51
Copy link
Contributor

@jamiezieziula jamiezieziula left a comment

Choose a reason for hiding this comment

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

There is a duplicate enabled map in the json schema. Also, can you remove all form: true fields in the schema, these aren't needed (we removed most before but must have missed these)

Removes the extra 'enabled' key from the schema as it's already defined
on line 599.
@mitchnielsen
Copy link
Contributor Author

mitchnielsen commented Jun 4, 2024

Removed form fields from the server schema in 88262c8.

@mitchnielsen mitchnielsen force-pushed the 336-postgresql-enabled-fixes branch from db29dcf to 88262c8 Compare June 4, 2024 20:44
@mitchnielsen
Copy link
Contributor Author

mitchnielsen commented Jun 4, 2024

  • Failures seem to be from hitting the Docker pull limit.
  • Will proceed with merge soon but wanted to give the community member a chance to chime in.
  • Will also consider if this could be a breaking change 🤔
    • It shouldn't be. Those who were using useSubChart can remove it, as that key is no longer considered. The existing key enabled now does what users expect it should do.

@pythonking6
Copy link

This is excellent!!!

@mitchnielsen mitchnielsen merged commit aaaa3ea into main Jun 5, 2024
13 of 16 checks passed
@mitchnielsen mitchnielsen deleted the 336-postgresql-enabled-fixes branch June 5, 2024 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Postgresql pod is still deployed even when setting postgresql.enabled=false
4 participants