Allow skipping PostgreSQL database backup/restore#353
Open
dsavineau wants to merge 1 commit into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (15)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughThis PR adds support for skipping PostgreSQL database data during backup and restore operations in the EDA server operator. The changes refactor database configuration retrieval into reusable task files, initialize backup directories early in the pipeline, and extend CRD schemas with the new ChangesPostgreSQL Skip-Data Control for Backup and Restore
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dsavineau
requested review from
jamesmarshall24,
lucas-benedito and
rooftopcellist
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
roles/backup/tasks/set_database_config.yml (1)
15-23: 💤 Low valueInconsistent default handling pattern compared to restore role.
Line 22 uses
default('unmanaged'|b64encode) | b64decodewhile the restore role (shown in context snippet) usesb64decode | default('unmanaged'). Both produce the same result, but the restore pattern is more conventional and easier to understand. Consider aligning with the restore implementation for consistency.♻️ Suggested alignment with restore pattern
- eda_postgres_type: "{{ pg_config['resources'][0]['data']['type'] | default('unmanaged'|b64encode) | b64decode }}" + eda_postgres_type: "{{ pg_config['resources'][0]['data']['type'] | b64decode | default('unmanaged') }}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@roles/backup/tasks/set_database_config.yml` around lines 15 - 23, The eda_postgres_type default is applied before b64decode which is inconsistent with the restore role; update the set_fact entry for eda_postgres_type in the Store Database Configuration task (the set_fact block) to decode first then apply default by changing its expression to apply b64decode before default (i.e., use pg_config['resources'][0]['data']['type'] | b64decode | default('unmanaged')) so it matches the restore pattern and is clearer.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@roles/backup/tasks/set_database_config.yml`:
- Around line 10-13: The fail task "Fail if postgres configuration secret status
does not exist" uses the wrong condition; change the when clause to check the
actual resources list inside pg_config (e.g., pg_config.resources) rather than
the top-level dict: update the when to something like testing whether
pg_config.resources is undefined or empty (for example using pg_config.resources
is not defined or pg_config.resources | default([]) | length == 0) so the
failure triggers when the secret is missing or the resources list is empty.
In `@roles/backup/tasks/update_status.yml`:
- Line 13: The backupPostgres status calculation is wrong because it only uses
postgres_skip_data; change it to mirror the actual execution condition used in
creation.yml by setting backupPostgres to true when either postgres data is not
skipped or the Postgres type is managed (i.e., use the boolean expression
equivalent to "not postgres_skip_data | bool or eda_postgres_type ==
'managed'"); update the variable expression referencing backupPostgres,
postgres_skip_data and eda_postgres_type so the status accurately reflects
whether the backup task ran.
In `@roles/restore/tasks/set_database_config.yml`:
- Around line 14-22: The set_fact task stores database fields by directly
indexing pg_config['resources'][0], which will error if the Secret wasn't found;
update the role to first check that pg_config.resources exists and has at least
one item (e.g., with a when: "pg_config.resources is defined and
pg_config.resources | length > 0") before running the set_fact that assigns
eda_postgres_user, eda_postgres_pass, eda_postgres_database, eda_postgres_port,
eda_postgres_host, and eda_postgres_type, and add an alternate path (either a
fail task with a clear message or a fallback set_fact with safe defaults using
default(...)) to handle the missing Secret case.
- Around line 14-22: The task assumes all keys exist in
pg_config['resources'][0]['data']; add explicit validation before set_fact to
check the required keys (username, password, database, port, host, type) are
present and provide a clear error if any are missing or else supply safe
defaults for optional values. Use the same pg_config reference in an assert or
fail task (or use the default filter for eda_postgres_type) to verify each key
exists (e.g., check "'username' in pg_config['resources'][0]['data']" etc.), and
only run the set_fact that decodes values (eda_postgres_user, eda_postgres_pass,
eda_postgres_database, eda_postgres_port, eda_postgres_host, eda_postgres_type)
after validation so failures produce a clear, actionable message.
In `@roles/restore/tasks/update_status.yml`:
- Line 11: The restorePostgres status is computed from postgres_skip_data only,
but the actual restore task (postgres.yml via main.yml) runs when "not
postgres_skip_data | bool or eda_postgres_type == 'managed'"; update the
calculation of restorePostgres to mirror that condition so the status reflects
whether the PostgreSQL restore actually ran — e.g. set restorePostgres to the
boolean expression "not postgres_skip_data | bool or eda_postgres_type ==
'managed'" (reference variables: restorePostgres, postgres_skip_data,
eda_postgres_type and the postgres.yml/main.yml condition).
---
Nitpick comments:
In `@roles/backup/tasks/set_database_config.yml`:
- Around line 15-23: The eda_postgres_type default is applied before b64decode
which is inconsistent with the restore role; update the set_fact entry for
eda_postgres_type in the Store Database Configuration task (the set_fact block)
to decode first then apply default by changing its expression to apply b64decode
before default (i.e., use pg_config['resources'][0]['data']['type'] | b64decode
| default('unmanaged')) so it matches the restore pattern and is clearer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 06e69410-8cce-4dab-a207-c86923ae5dd3
📒 Files selected for processing (15)
config/crd/bases/eda.ansible.com_edabackups.yamlconfig/crd/bases/eda.ansible.com_edarestores.yamlconfig/manifests/bases/eda-server-operator.clusterserviceversion.yamlroles/backup/defaults/main.ymlroles/backup/tasks/creation.ymlroles/backup/tasks/init.ymlroles/backup/tasks/postgres.ymlroles/backup/tasks/set_database_config.ymlroles/backup/tasks/update_status.ymlroles/restore/defaults/main.ymlroles/restore/tasks/check_postgres_backup.ymlroles/restore/tasks/main.ymlroles/restore/tasks/postgres.ymlroles/restore/tasks/set_database_config.ymlroles/restore/tasks/update_status.yml
💤 Files with no reviewable changes (2)
- roles/restore/tasks/postgres.yml
- roles/backup/tasks/postgres.yml
This allows one to skip the PostgreSQL database dump/restore during via the EDABackup and EDARestore CRDs on external database configuration. A new parameter called `postgres_skip_data` is introduced (defaults to false for backward compatibility) but can be turn on to skip the pg_dump and pg_restore parts. That value is passed down to nested components so other pg_dump/pg_restore calls are also skipped. At restore time, if no database dump file is found in the backup then the `postgres_skip_data` parameter needs to be explicitely set to true. Signed-off-by: Dimitri Savineau <dsavinea@redhat.com>
dsavineau
force-pushed
the
skip_db_backup_restore
branch
from
ef4e164 to
5680468
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This allows one to skip the PostgreSQL database dump/restore during via the EDABackup and EDARestore CRDs on external database configuration.
A new parameter called
postgres_skip_datais introduced (defaults to false for backward compatibility) but can be turn on to skip the pg_dump and pg_restore parts.At restore time, if no database dump file is found in the backup then the
postgres_skip_dataparameter needs to be explicitely set to true.Summary by CodeRabbit
postgres_skip_dataoption to skip PostgreSQL database dump/restore data in both backup and restore operations.status.backupPostgresand.status.restorePostgresfields to report whether PostgreSQL data handling was performedpostgres_skip_datawhen it’s missing