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
WalkthroughAdds an optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
According to the velero code definition: // DefaultVolumesToFsBackup specifies whether pod volume file system backup should be used // for all volumes by default. // +optional // +nullable DefaultVolumesToFsBackup *bool `json:"defaultVolumesToFsBackup,omitempty"` Signed-off-by: Qixuan Wang <qwang@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ocp_resources/backup.py (1)
24-32: Add missing docstring entry for the new constructor parameter.
default_volumes_to_fs_backupwas added to__init__but is not documented in the Args section yet.📝 Proposed docstring patch
Args: included_namespaces (list, optional): Namespaces to include in the backup. If unspecified, all namespaces are included. excluded_resources (list, optional): Resources to exclude from the backup. Resources may be shortcuts (e.g. 'po' for 'pods') or fully-qualified. 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 | None, optional): Controls whether pod volumes + are backed up with file-system backup by default. If None, the field is omitted. 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 24 - 32, The docstring for the class constructor is missing documentation for the new parameter default_volumes_to_fs_backup; update the Args section in the __init__ docstring to add an entry describing default_volumes_to_fs_backup (type: bool, optional) and its behavior (e.g., whether volumes default to filesystem backup vs block, default value, and effect on snapshot_move_data or DataMover behavior) so callers understand the new parameter semantics and default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ocp_resources/backup.py`:
- Around line 24-32: The docstring for the class constructor is missing
documentation for the new parameter default_volumes_to_fs_backup; update the
Args section in the __init__ docstring to add an entry describing
default_volumes_to_fs_backup (type: bool, optional) and its behavior (e.g.,
whether volumes default to filesystem backup vs block, default value, and effect
on snapshot_move_data or DataMover behavior) so callers understand the new
parameter semantics and default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e4e1da38-485e-491e-83f4-efb9a1267bc6
📒 Files selected for processing (1)
ocp_resources/backup.py
rnetser
requested changes
Mar 4, 2026
Collaborator
rnetser
left a comment
rnetser
left a comment
There was a problem hiding this comment.
please use the class-generator to generate the resource
Contributor
Author
|
@rnetser How to generate will generate Resource.ApiGroup.POSTGRESQL_CNPG_NOOBAA_IO, which is not my target. Then I did: Schema was updated successfully. Checking mapping, I got Since no mapping for Backup, why did it could generate backups.postgresql.cnpg.noobaa.io? If I tried different ways: Seems class-generator doesn't support -k CRD. I exported backups.velero.io and put it under class_generator/schema/velero.io_Backup.json Seems class-generator doesn't support using specific schema. Where did I go wrong? |
7 tasks
Contributor
Author
|
@rnetser Is it possible to merge the tiny change and then I will open a new PR to use class generator for the whole backup.py? |
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
5 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.
According to the velero code definition:
// DefaultVolumesToFsBackup specifies whether pod volume file system backup should be used
// for all volumes by default.
// +optional
// +nullable
DefaultVolumesToFsBackup *bool
json:"defaultVolumesToFsBackup,omitempty"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