feat: Add defaultVolumesToFsBackup field to Backup#2654
Open
qwang1 wants to merge 1 commit intoRedHatQE:mainfrom
Open
feat: Add defaultVolumesToFsBackup field to Backup#2654qwang1 wants to merge 1 commit intoRedHatQE:mainfrom
qwang1 wants to merge 1 commit intoRedHatQE:mainfrom
Conversation
If defaultVolumesToFsBackup is not set, it equals to the default value False Signed-off-by: Qixuan Wang <qwang@redhat.com>
WalkthroughAdded an optional parameter Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
Contributor
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ocp_resources/backup.py (1)
23-32:⚠️ Potential issue | 🟡 MinorAdd docstring entry for the new
default_volumes_to_fs_backupparameter.The Args block documents every other parameter but omits
default_volumes_to_fs_backup.📝 Proposed fix
snapshot_move_data (bool, optional): If set to true, deploys the volume snapshot mover controller and a modified CSI Data Mover plugin. + default_volumes_to_fs_backup (bool, optional): If set to true, all pod volumes are backed + up via file system backup by default. storage_location (string, optional): Define the location for the DataMover.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ocp_resources/backup.py` around lines 23 - 32, The docstring Args block is missing an entry for the new parameter default_volumes_to_fs_backup; update the existing docstring in ocp_resources/backup.py (the function/class docstring that currently lists included_namespaces, excluded_resources, snapshot_move_data, storage_location) to add a line describing default_volumes_to_fs_backup (bool, optional): explain its default behavior and what enabling it does (e.g., whether volumes are backed up as filesystem snapshots by default), matching the style of the other parameter entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ocp_resources/backup.py`:
- Around line 59-61: In to_dict() fix the unconditional emission of
spec_dict["defaultVolumesToFsBackup"] — instead of always writing False when
self.default_volumes_to_fs_backup is None, only set
spec_dict["defaultVolumesToFsBackup"] when self.default_volumes_to_fs_backup is
not None (so an explicit False is preserved but None omits the key); locate the
logic around the default_volumes_to_fs_backup attribute in the to_dict() method
and change the assignment to a conditional presence check before adding the
"defaultVolumesToFsBackup" key.
---
Outside diff comments:
In `@ocp_resources/backup.py`:
- Around line 23-32: The docstring Args block is missing an entry for the new
parameter default_volumes_to_fs_backup; update the existing docstring in
ocp_resources/backup.py (the function/class docstring that currently lists
included_namespaces, excluded_resources, snapshot_move_data, storage_location)
to add a line describing default_volumes_to_fs_backup (bool, optional): explain
its default behavior and what enabling it does (e.g., whether volumes are backed
up as filesystem snapshots by default), matching the style of the other
parameter entries.
Comment on lines
+59
to
+61
| spec_dict["defaultVolumesToFsBackup"] = ( | ||
| self.default_volumes_to_fs_backup if self.default_volumes_to_fs_backup is not None else False | ||
| ) |
There was a problem hiding this comment.
defaultVolumesToFsBackup is always emitted, even when the caller never set it — inconsistent with all other optional fields.
Every other optional field in to_dict() is guarded by a truthiness or existence check and is omitted from the spec when not provided. This field unconditionally writes defaultVolumesToFsBackup: false whenever default_volumes_to_fs_backup is None, which can silently override a server-side or cluster-level default that differs from false.
The bool | None = None signature already signals "this field is optional"; the serialisation should honour that by only emitting the key when the caller explicitly provides a value (including an explicit False).
♻️ Proposed fix
- spec_dict["defaultVolumesToFsBackup"] = (
- self.default_volumes_to_fs_backup if self.default_volumes_to_fs_backup is not None else False
- )
+ if self.default_volumes_to_fs_backup is not None:
+ spec_dict["defaultVolumesToFsBackup"] = self.default_volumes_to_fs_backup🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ocp_resources/backup.py` around lines 59 - 61, In to_dict() fix the
unconditional emission of spec_dict["defaultVolumesToFsBackup"] — instead of
always writing False when self.default_volumes_to_fs_backup is None, only set
spec_dict["defaultVolumesToFsBackup"] when self.default_volumes_to_fs_backup is
not None (so an explicit False is preserved but None omits the key); locate the
logic around the default_volumes_to_fs_backup attribute in the to_dict() method
and change the assignment to a conditional presence check before adding the
"defaultVolumesToFsBackup" key.
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
3 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.
If defaultVolumesToFsBackup is not set, it equals to the default value False
Short description:
More details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
Bug:
Summary by CodeRabbit