Refactor PostgreSQL configuration and remove deprecated database setup #215
Conversation
- Introduced a unified PostgreSQL configuration structure in values.yaml, replacing the old db configuration. - Added new helper functions for managing PostgreSQL environment variables and secrets based on the selected configuration type (postgrescluster, external-plaintext, external-secret). - Removed old database-related templates (ConfigMap, Deployment, PVC, Secrets, Service) that are no longer needed. - Updated the pgstacbootstrap job and configmap templates to align with the new PostgreSQL configuration. - Implemented validation for PostgreSQL settings to ensure required fields are set based on the selected type.
…ude DATABASE_URL for connection string
emmanuelmathot
requested review from
pantierra,
Copilot,
batpad,
sunu and
ividito
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the PostgreSQL configuration to provide a unified, standardized approach for managing database access while removing deprecated configuration sections. Key changes include the removal of the old db section in values.yaml, the addition of a new postgresql configuration section with multiple connection types, and updates to deployments and bootstrap jobs to use new helper functions and standardized environment variables.
Reviewed Changes
Copilot reviewed 4 out of 10 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| helm-chart/eoapi/values.yaml | Removed deprecated db section and introduced unified PostgreSQL config |
| helm-chart/eoapi/templates/services/deployment.yaml | Updated to use new validation and environment variable injection |
| helm-chart/eoapi/templates/pgstacbootstrap/job.yaml | Refactored job configuration to align with new PostgreSQL environment |
| helm-chart/eoapi/templates/pgstacbootstrap/configmap.yaml | Simplified conditional logic for fixture loading in the ConfigMap |
Files not reviewed (6)
- helm-chart/eoapi/templates/_helpers.tpl: Language not supported
- helm-chart/eoapi/templates/db/configmap.yaml: Language not supported
- helm-chart/eoapi/templates/db/deployment.yaml: Language not supported
- helm-chart/eoapi/templates/db/pvc.yaml: Language not supported
- helm-chart/eoapi/templates/db/secrets.yaml: Language not supported
- helm-chart/eoapi/templates/db/service.yaml: Language not supported
Comments suppressed due to low confidence (2)
helm-chart/eoapi/templates/pgstacbootstrap/job.yaml:89
- The removal of the condition checking 'eq .Values.pgstacBootstrap.settings.envVars.LOAD_FIXTURES "true"' may change the behavior for loading fixtures. Please verify that this change is intentional.
{{- if and .Values.pgstacBootstrap.enabled .Values.pgstacBootstrap.settings.loadSamples }}
helm-chart/eoapi/templates/pgstacbootstrap/configmap.yaml:18
- The simplified conditional for fixture loading removes the check for 'LOAD_FIXTURES' from envVars. Confirm that this change aligns with the intended behavior.
{{- if .Values.pgstacBootstrap.settings.loadSamples }}
| # Optional: if these are provided in the secret | ||
| host: "host" | ||
| port: "port" | ||
| database: "database" |
There was a problem hiding this comment.
Should we add a comment here to clarify that these values override external.host, external.port and external.database if defined?
| {{- if or $.Values.postgrescluster.enabled (eq $.Values.postgresql.type "postgrescluster") }} | ||
| {{- include "eoapi.postgresqlEnv" $ | nindent 12 }} | ||
| {{- else if eq $.Values.postgresql.type "external-plaintext" }} | ||
| {{- include "eoapi.postgresqlEnv" $ | nindent 12 }} | ||
| {{- else if eq $.Values.postgresql.type "external-secret" }} | ||
| {{- include "eoapi.postgresqlEnv" $ | nindent 12 }} | ||
| {{- end }} |
There was a problem hiding this comment.
Looks like the contents of the if/else block is same in all 3 cases and for any variations we have full control over how eoapi.postgresqlEnv is defined. Should we consider consolidating this into a single block?
| {{/* | ||
| Map legacy configuration to new postgresql configuration | ||
| */}} | ||
| {{- define "eoapi.mapLegacyPostgresql" -}} |
There was a problem hiding this comment.
Might be missing something obvious, but I don’t see an include statement that uses this anywhere. Since we’re already doing some cleanup and breaking changes, think it’s ok to drop this?
Overview
This PR implements the unified PostgreSQL configuration as proposed in issue #209. It provides a cleaner, more standardized approach to managing PostgreSQL database access across different deployment scenarios.
Changes
Added new
postgresqlconfiguration section in values.yaml with three supported types:postgrescluster: Default using CrunchyData PGOexternal-plaintext: For user-managed PostgreSQL with direct credentialsexternal-secret: For user-managed PostgreSQL with secret referencesStandardized environment variables to use official PostgreSQL variables:
PGHOST,PGPORT,PGUSER,PGPASSWORD,PGDATABASEPOSTGRES_*variables maintained for backward compatibilityAdded new helper functions in _helpers.tpl:
eoapi.postgresqlEnv: Main function for handling all PostgreSQL configurationseoapi.validatePostgresql: Validation for PostgreSQL configurationeoapi.mapLegacyPostgresql: Backward compatibility mappingRemoved deprecated
dbsection from values.yamlUpdated service deployments and pgstacbootstrap jobs to use the new configuration
Benefits
Breaking Changes
dbsection from values.yamlMigration
Users should update their values.yaml to use the new
postgresqlsection instead of the deprecateddbsection: