refactor: update DataVolume resource#2480
Conversation
|
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
Container Operations
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. |
|
Caution Review failedThe pull request is closed. WalkthroughDataVolume in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 💡 Knowledge Base configuration:
You can enable these settings in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ocp_resources/datavolume.py(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rnetser
PR: RedHatQE/openshift-python-wrapper#2387
File: ocp_resources/user.py:45-47
Timestamp: 2025-04-28T12:52:11.677Z
Learning: In the openshift-python-wrapper, resources can be created either through constructor parameters or from YAML files. The `to_dict` method in resource classes typically checks for `self.kind_dict` and `self.yaml_file` to determine if parameters need to be explicitly provided. Parameters that would otherwise be required can be omitted when creating from YAML.
Learnt from: myakove
PR: RedHatQE/openshift-python-wrapper#2171
File: ocp_resources/mariadb_operator.py:90-140
Timestamp: 2024-10-15T13:51:36.142Z
Learning: In `ocp_resources/mariadb_operator.py`, the `to_dict` method in the `MariadbOperator` class is intentionally kept in its current form for better readability, and should not be refactored to reduce repetition.
Learnt from: myakove
PR: RedHatQE/openshift-python-wrapper#2209
File: ocp_resources/volume_snapshot_class.py:0-0
Timestamp: 2024-12-03T08:02:11.880Z
Learning: In generated code files like `ocp_resources/volume_snapshot_class.py`, avoid suggesting adding validation checks or modifications, as generated code should not be manually altered.
Learnt from: myakove
PR: RedHatQE/openshift-python-wrapper#2184
File: ocp_resources/user_defined_network.py:143-168
Timestamp: 2024-10-30T09:40:53.905Z
Learning: In `ocp_resources/user_defined_network.py`, for type hints of parameters like `subnets` that are lists of dictionaries, prefer using `Optional[List[Dict[str, Any]]]` instead of defining a `TypedDict`.
📚 Learning: in generated code files like `ocp_resources/volume_snapshot_class.py`, avoid suggesting adding valid...
Learnt from: myakove
PR: RedHatQE/openshift-python-wrapper#2209
File: ocp_resources/volume_snapshot_class.py:0-0
Timestamp: 2024-12-03T08:02:11.880Z
Learning: In generated code files like `ocp_resources/volume_snapshot_class.py`, avoid suggesting adding validation checks or modifications, as generated code should not be manually altered.
Applied to files:
ocp_resources/datavolume.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: conventional-title
- GitHub Check: python-module-install
- GitHub Check: tox
🔇 Additional comments (1)
ocp_resources/datavolume.py (1)
76-150: Constructor refactoring looks good!The addition of type annotations and the use of
**kwargspattern aligns with the framework's design where resources can be created either through constructor parameters or from YAML files.
jpeimer
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
ocp_resources/datavolume.py (2)
105-128: Tighten types and docs for access modes, volume mode, api_name, and source
- K8s expects accessModes as a list of strings. Current API takes a single str and wraps it into a one-item list. Consider accepting list[str] too.
- Consider Literal typing for
sourceandapi_nameto prevent invalid values at type-check time.Apply these adjustments to signature and docs:
@@ - access_modes: str | None = None, - volume_mode: str | None = None, + access_modes: str | list[str] | None = None, + volume_mode: str | None = None, @@ - access_modes (str, default: None): DataVolume access mode (e.g., "ReadWriteOnce", "ReadWriteMany"). + access_modes (str | list[str], default: None): DataVolume access mode(s), K8s expects a list. + Examples: "ReadWriteOnce" or ["ReadWriteOnce","ReadOnlyMany"]. @@ - api_name (str, default: "pvc"): api used for DV (e.g., "storage", "pvc"). + api_name (str, default: "pvc"): API used for DV ("pvc" or "storage").And in to_dict:
@@ - if self.access_modes is not None: - _spec[self.api_name]["accessModes"] = [self.access_modes] + if self.access_modes is not None: + _spec[self.api_name]["accessModes"] = ( + self.access_modes if isinstance(self.access_modes, list) else [self.access_modes] + )
173-189: Avoid creating an empty pvc/storage block when no fields are setCurrently,
_spec[self.api_name] = {}is added wheneverapi_nameis not None, even if none of accessModes/volumeMode/storageClass/size are provided. This produces an empty object in the manifest, which is unnecessary and can confuse readers or validators.Refine to create the block only when needed:
- if self.api_name is not None: - _spec[self.api_name] = {} + if any([self.access_modes, self.volume_mode, self.storage_class, self.size]): + api_name = self.api_name or "pvc" + _spec[api_name] = {} @@ - if self.access_modes is not None: - _spec[self.api_name]["accessModes"] = [self.access_modes] + if self.access_modes is not None: + _spec[api_name]["accessModes"] = ( + self.access_modes if isinstance(self.access_modes, list) else [self.access_modes] + ) @@ - if self.volume_mode is not None: - _spec[self.api_name]["volumeMode"] = self.volume_mode + if self.volume_mode is not None: + _spec[api_name]["volumeMode"] = self.volume_mode @@ - if self.storage_class is not None: - _spec[self.api_name]["storageClassName"] = self.storage_class + if self.storage_class is not None: + _spec[api_name]["storageClassName"] = self.storage_class @@ - if self.size is not None: - _spec[self.api_name]["resources"] = {"requests": {"storage": self.size}} + if self.size is not None: + _spec[api_name]["resources"] = {"requests": {"storage": self.size}}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ocp_resources/datavolume.py(4 hunks)
🔇 Additional comments (3)
ocp_resources/datavolume.py (3)
211-212: LGTM: sourceRef support is clearly separatedUsing
sourceRefas a separate path keepssourceandsourceReforthogonal. No issues spotted.
214-228: LGTM: annotations use setdefault + update safely
provisionOnNode, Multus networks, and bind-immediate annotations are set safely withsetdefault.- Keys are correctly namespaced.
76-100: No backward-compatibility issues detected for DataVolume constructorI ran the provided script across the repo and found zero calls to
DataVolume(...)using positional arguments (or otherwise). Since there are no existing instantiations in the codebase, the refactored__init__signature poses no breakage risk.No changes required.
|
/verified |
…rapper into update_dv_resource
|
/verified |
…rapper into update_dv_resource
|
/verified |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
ocp_resources/datavolume.py (2)
175-190: Avoid emitting an empty pvc/storage block; initialize nested dicts safely.Currently
_spec[self.api_name] = {}is created even when no inner fields are provided, which can lead to an invalid DV spec likepvc: {}. Initialize only when any nested field is set, and use setdefault for resources.- # Set api_name spec fields (pvc/storage) - if self.api_name is not None: - _spec[self.api_name] = {} + # Set api_name spec fields (pvc/storage) only when needed + if self.api_name is not None and any([self.access_modes, self.volume_mode, self.storage_class, self.size]): + _spec[self.api_name] = {} if self.access_modes is not None: - _spec[self.api_name]["accessModes"] = [self.access_modes] + _spec[self.api_name]["accessModes"] = ( + [self.access_modes] if isinstance(self.access_modes, str) else list(self.access_modes) + ) if self.volume_mode is not None: _spec[self.api_name]["volumeMode"] = self.volume_mode if self.storage_class is not None: _spec[self.api_name]["storageClassName"] = self.storage_class if self.size is not None: - _spec[self.api_name]["resources"] = {"requests": {"storage": self.size}} + _spec[self.api_name].setdefault("resources", {}).setdefault("requests", {})["storage"] = self.size
191-211: Constrain secretRef/certConfigMap to http/registry sources to avoid invalid specs.Attaching these to pvc/upload/blank produces invalid fields and may be rejected by the API. Also guards against KeyError for unknown sources.
- if self.secret is not None: - source_spec[self.source]["secretRef"] = self.secret.name - if self.cert_configmap is not None: - source_spec[self.source]["certConfigMap"] = self.cert_configmap + # Credentials and certs are applicable only for http/registry sources + if self.source in ("http", "registry"): + if self.secret is not None: + source_spec[self.source]["secretRef"] = self.secret.name + if self.cert_configmap is not None: + source_spec[self.source]["certConfigMap"] = self.cert_configmap
🧹 Nitpick comments (1)
ocp_resources/datavolume.py (1)
80-102: Constructor refactor and explicit typing improve clarity. Consider supporting multiple access modes.DV spec allows a list of accessModes. If you want to support both single and multiple values without burdening callers, accept str | list[str] and normalize in to_dict.
Apply this diff to the signature:
- access_modes: str | None = None, + access_modes: str | list[str] | None = None,And adjust emission (see lines 179-181 change below).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
ocp_resources/datavolume.py(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: jpeimer
PR: RedHatQE/openshift-python-wrapper#2480
File: ocp_resources/datavolume.py:158-172
Timestamp: 2025-08-10T13:55:36.406Z
Learning: In CDI v1beta1 DataVolume resources, the `priorityClassName` field is placed directly under `spec` (i.e., `spec.priorityClassName`), not under `spec.podSettings`. The field `spec.podSettings` does not exist in CDI v1beta1. This applies to the file `ocp_resources/datavolume.py` in the openshift-python-wrapper repository.
📚 Learning: 2025-08-10T13:55:36.406Z
Learnt from: jpeimer
PR: RedHatQE/openshift-python-wrapper#2480
File: ocp_resources/datavolume.py:158-172
Timestamp: 2025-08-10T13:55:36.406Z
Learning: In CDI v1beta1 DataVolume resources, the `priorityClassName` field is placed directly under `spec` (i.e., `spec.priorityClassName`), not under `spec.podSettings`. The field `spec.podSettings` does not exist in CDI v1beta1. This applies to the file `ocp_resources/datavolume.py` in the openshift-python-wrapper repository.
Applied to files:
ocp_resources/datavolume.py
📚 Learning: 2025-08-10T13:55:31.630Z
Learnt from: jpeimer
PR: RedHatQE/openshift-python-wrapper#2480
File: ocp_resources/datavolume.py:189-209
Timestamp: 2025-08-10T13:55:31.630Z
Learning: The openshift-python-wrapper project is designed as a thin wrapper around the Kubernetes/OpenShift API and intentionally does not perform input validation. Validation is left to the API server itself. Do not suggest adding ValueError exceptions or input validation checks in resource classes like DataVolume, VirtualMachine, etc.
Applied to files:
ocp_resources/datavolume.py
🧬 Code Graph Analysis (1)
ocp_resources/datavolume.py (2)
ocp_resources/secret.py (1)
Secret(5-73)ocp_resources/resource.py (5)
to_dict(734-738)to_dict(1641-1643)update(1069-1087)update(1689-1736)NamespacedResource(1533-1643)
🔇 Additional comments (8)
ocp_resources/datavolume.py (8)
1-2: Good call using postponed evaluation of annotations.This avoids runtime imports for type hints and plays well with TYPE_CHECKING.
15-19: TYPE_CHECKING import for Secret is correct and lightweight.Keeps runtime deps minimal while preserving type safety.
107-130: Docstring updates are clear and helpful.Coverage of new fields (source_dict, source_ref, checkpoints, final_checkpoint, priority_class_name) looks accurate.
131-153: **Switch to super().init(kwargs) is appropriate.Keeps the wrapper thin and consistent with other resources.
154-159: Spec init only when building from params is correct.The guard with kind_dict/yaml_file aligns with project conventions.
160-174: Top-level spec fields LGTM; priorityClassName placement is correct for CDI v1beta1.Setting checkpoints, contentType, finalCheckpoint, preallocation, and priorityClassName directly under spec matches the DV schema. The priorityClassName location aligns with our retrieved learning for CDI v1beta1.
213-214: sourceRef handling is clean and non-intrusive.Coexists nicely with source/source_dict for consumers that prefer indirect references.
216-229: Annotation handling looks correct.
- provisionOnNode under kubevirt.io is properly keyed
- multus networks annotation is right
- bind.immediate.requested uses the CDI api_group prefix as expected
|
/verified |
|
/approve |
|
/cherry-pick v4.19 |
|
Cherry-pick branch created, but PR creation failed |
7 participants
Summary by CodeRabbit
New Features
Refactor
Bug Fixes