fix(prefect-server): better support for internal and external database configs

[mitchnielsen]

Summary

This will be a breaking change, so we should release this in a new major version. The configuration keys have moved around. If we really need to, we can try to build in some backwards-compatibility, but that always ends up looking pretty rough in Helm.

Ensures that the PostgreSQL Secret is created, even if postgresql.enabled=false. This ensures that we support a use case where folks want to use an external instance of PostgreSQL, but still want the Secret to automatically be generated with the proper connection string.

With a recent change, we would skip creation of this secret if PostgreSQL was disabled which forced users to create a Secret themselves. This change now allows them to continue providing the auth values and letting the chart build the Secret with the correct connection string.

I ended up needing to go a step further by removing any custom configuration that was injected under the postgresql key, which should be dedicated to overriding default values from the PostgreSQL chart. There's now a new secret key, which aligns with the other top-level keys (deployment, service, etc.). These values will be considered if postgresql.enabled=false.

Closes https://github.com/PrefectHQ/prefect-helm/issues/358

Follow-up to https://github.com/PrefectHQ/prefect-helm/pull/341.

Testing

See the added test suite.

[mitchnielsen]

@ahgraber @tmyhu @MRocholl - I believe this should cover all the use cases you mentioned. Could you please give this a look when you have a moment and let me know if this needs any tweaking?

Trying to make it flexible enough to cover all use cases while keeping it as consistent/straightforward as possible. Thanks!

[tmyhu]

@mitchnielsen it looks good to me, but won't make any difference for our use case since we are using SQLite. I assume that's not something you really want to support specifically since there is no hint about it anywhere in the helm chart I could see? We should probably migrate to PostgreSQL anyway, but for now I'm happy to continue creating the secret and connection string manually for SQLite.

[ahgraber]

@mitchnielsen Thanks for this effort!

The output connection string from the test deployment's secret is missing the hostname in the database address; will that still be pulled in from postgresql.externalHostname? If so, I believe this covers the requested use case.

[mitchnielsen]

it looks good to me, but won't make any difference for our use case since we are using SQLite. I assume that's not something you really want to support specifically since there is no hint about it anywhere in the helm chart I could see? We should probably migrate to PostgreSQL anyway, but for now I'm happy to continue creating the secret and connection string manually for SQLite.

@tmyhu fair point. We have this in mind and are tracking in https://github.com/PrefectHQ/prefect-helm/issues/345. I'd like to provide the auth configuration outside of the context of the postgresql key to make the configuration more clear, and open the door for supporting SQLite since we provide that information in our self-hosted docs. That said, moving to PostgreSQL is probably the ideal longer-term move.

[mitchnielsen]

@ahgraber great call-out, let me see if I can make a minimal change to make that configuration supported. If not, may need to jump ahead to providing this config outside of the postgresql key because of how some of the nested templates are used.

[mitchnielsen]

@ahgraber @tmyhu @MRocholl - alrighty, bit of a rework here to make sure we have proper support for either internal or external database configuration. For some examples of the configuration you'd provide, check out the updated docs or the unit test file. Welcoming any feedback here 🤝

[MRocholl]

@mitchnielsen sorry for the late reply. These changes should satisfy the requirements I believe 👍 Thx for the great work

[mitchnielsen]

sorry for the late reply. These changes should satisfy the requirements I believe 👍 Thx for the great work

@MRocholl - no worries at all, thank you for confirming 🤝