refactor: update DataVolume resource with type hints, source_dict support, and improved spec construction

[ema-aka-young]

Short description:

From the original PR #2480 by @jpeimer

• Added checkpoints, final-checkpoint flag, priority class name, explicit source references, and broader source types with optional secret/cert support.
• Hardened and clarified DataVolume constructor with explicit typed parameters.
• Modularized spec construction for clearer, more maintainable resource output.
• Removed legacy/deprecated annotation and path handling to avoid misuse.

More details:

What this PR does / why we need it:

Main reason is that this contains changes that are useful to support the datasource usage on 4.18.

Which issue(s) this PR fixes:

First part of CNV-88559

Special notes for reviewer:

This is a manually cherry-pick of #2723.

Bug:

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: f772272d-e0b7-4438-ba0b-d0e034ddd40b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[redhat-qe-bot1]

Report bugs in Issues

Welcome! 🎉

This pull request will be automatically processed with the following features:

🔄 Automatic Actions

• Reviewer Assignment: Reviewers are automatically assigned based on the OWNERS file in the repository root
• Size Labeling: PR size labels (XS, S, M, L, XL, XXL) are automatically applied based on changes
• Issue Creation: Disabled for this repository
• Branch Labeling: Branch-specific labels are applied to track the target branch
• Auto-verification: Auto-verified users have their PRs automatically marked as verified
• Labels: All label categories are enabled (default configuration)

📋 Available Commands

PR Status Management

• /wip - Mark PR as work in progress (adds WIP: prefix to title)
• /wip cancel - Remove work in progress status
• /hold - Block PR merging (PR author or approvers)
• /hold cancel - Unblock PR merging (PR author or approvers)
• /verified - Mark PR as verified
• /verified cancel - Remove verification status
• /reprocess - Trigger complete PR workflow reprocessing (useful if webhook failed or configuration changed)
• /regenerate-welcome - Regenerate this welcome message
• /security-override - Set security check runs to pass (maintainers only)
• /security-override cancel - Re-run security checks

Review & Approval

• /lgtm - Approve changes (looks good to me)
• /approve - Approve PR (approvers only)
• /automerge - Enable automatic merging when all requirements are met (maintainers and approvers only)
• /assign-reviewers - Assign reviewers based on OWNERS file
• /assign-reviewer @username - Assign specific reviewer
• /check-can-merge - Check if PR meets merge requirements

Testing & Validation

• /retest tox - Run Python test suite with tox
• /retest python-module-install - Test Python package installation
• /retest conventional-title - Validate commit message format
• /retest all - Run all available tests

Cherry-pick Operations

• /cherry-pick <branch> - Schedule cherry-pick to target branch when PR is merged
  • Multiple branches: /cherry-pick branch1 branch2 branch3
• /cherry-pick-retry <branch> - Retry a failed cherry-pick (merged PRs only)

Branch Management

• /rebase - Rebase this PR branch onto its base branch

Label Management

• /<label-name> - Add a label to the PR
• /<label-name> cancel - Remove a label from the PR

✅ Merge Requirements

This PR will be automatically approved when the following conditions are met:

1. Approval: /approve from at least one approver
2. Status Checks: All required status checks must pass
3. No Blockers: No wip, hold, has-conflicts labels and PR must be mergeable (no conflicts)
4. Verified: PR must be marked as verified

📊 Review Process

Approvers and Reviewers

Approvers:

• myakove
• rnetser

Reviewers:

• dbasunag
• myakove
• rnetser

Available Labels

• hold
• verified
• wip
• lgtm
• approve
• automerge

AI Features

• Conventional Title: Mode: fix (claude/claude-opus-4-6-1m)
• Cherry-Pick Conflict Resolution: Enabled (claude/claude-opus-4-6-1m)

Security Checks

• Suspicious Path Detection: Monitors paths: .claude/, .vscode/, .cursor/, .devcontainer/, .pi/, .github/workflows/, .github/actions/
• Committer Identity Check: Verifies last committer matches PR author
• Mandatory: Security checks block merge (use /security-override to bypass — maintainers only)

💡 Tips

• WIP Status: Use /wip when your PR is not ready for review
• Verification: The verified label is removed on new commits unless the push is detected as a clean rebase
• Cherry-picking: Cherry-pick labels are processed when the PR is merged
• Permission Levels: Some commands require approver permissions
• Auto-verified Users: Certain users have automatic verification and merge privileges

For more information, please refer to the project documentation or contact the maintainers.

[qodo-code-review]

PR Summary by Qodo

Refactor DataVolume spec generation; add sourceRef/checkpoints support for 4.18

✨ Enhancement 🐞 Bug fix 🕐 20-40 Minutes

AI Description

• Add DataVolume spec support for checkpoints, finalCheckpoint, priorityClassName, and sourceRef.
• Replace monolithic spec construction with typed parameters and explicit, modular to_dict building.
• Deprecate legacy source in favor of source_dict and fix incorrect source/url handling.

Diagram

graph TD
  A(["Caller / tests"]) --> B["DataVolume ctor"] --> C["to_dict spec builder"] --> D["DataVolume manifest"] --> E{{"K8s API"}}
  C --> F["Source / SourceRef"] --> D
  C --> G["PVC/Storage spec"] --> D
  C --> H["Annotations & extras"] --> D

  subgraph Legend
    direction LR
    _actor(["Actor"]) ~~~ _proc["Process"] ~~~ _ext{{"External"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Typed source builders (e.g., HttpSource/PvcSource/DataSourceRef classes)

• ➕ Stronger validation (mutual exclusivity, required fields like url) before manifest generation
• ➕ Clearer IDE discoverability and safer evolution as CDI adds more source variants
• ➖ Larger public API change and more files/classes to maintain
• ➖ Harder to backport safely to a stable branch if consumers depend on current kwargs-based patterns

2. Factory constructors per source type (from_http/from_pvc/from_datasource)

• ➕ Guides users toward correct parameter combinations without complex type hierarchies
• ➕ Keeps to_dict simpler by reducing branching
• ➖ More surface area (multiple entry points) and potential duplication
• ➖ Still needs a deprecation strategy for existing callers using source/source_dict directly

Recommendation: For a v4.18 backport, the PR’s incremental refactor (typed params + source_dict/sourceRef support with a deprecation warning) is the most practical approach. As a follow-up (not necessarily in this PR), consider adding explicit validation for incompatible combinations (source_dict vs source vs source_ref) and required fields (e.g., url for http/registry) to reduce runtime misconfiguration.

Files changed (1) +117 / -85

Enhancement (1) +117 / -85

datavolume.pyRefactor DataVolume constructor and spec generation; add 4.18 datasource fields +117/-85

Refactor DataVolume constructor and spec generation; add 4.18 datasource fields

• Introduces typed constructor parameters (including checkpoints/finalCheckpoint/priorityClassName and source_ref) and forwards remaining options via **kwargs. Reworks to_dict() to build spec fields modularly, supports explicit source_dict while deprecating the legacy source parameter, and fixes prior incorrect source/url branching logic.

ocp_resources/datavolume.py

[myakove]

/lgtm
/approve

[myakove]

/qodo do not review this PR

[qodo-code-review]

do not review this PR

Understood — I won’t review this PR.

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (3) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 18 rules

1. KeyError for unknown source 🐞 Bug ☼ Reliability

Description

DataVolume.to_dict() only initializes source_spec[self.source] for a limited set of source
values, but still unconditionally writes secretRef/certConfigMap under
source_spec[self.source]. For any other (deprecated) source value with secret or
cert_configmap set, this raises KeyError and prevents manifest generation.

Code

ocp_resources/datavolume.py[R204-217]

+                if self.source in ["http", "registry"]:
+                    source_spec[self.source] = {"url": self.url}
+                elif self.source in ["upload", "blank"]:
+                    source_spec[self.source] = {}
+                elif self.source == "pvc":
+                    source_spec[self.source] = {
+                        "name": self.source_pvc,
+                        "namespace": self.source_namespace or self.namespace,
+                    }
+
+                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

Relevance

⭐⭐⭐ High

They previously accepted fixes to initialize nested spec dicts to avoid KeyError in DataVolume
serialization.

PR-#2480

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code only creates source_spec[self.source] inside the handled if/elif chain; if
self.source is anything else, the subsequent source_spec[self.source]["secretRef"] /
["certConfigMap"] indexing will raise KeyError.

ocp_resources/datavolume.py[191-217]
PR-#2480

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`DataVolume.to_dict()` may raise `KeyError` when `source` is provided (deprecated path), is not one of the explicitly handled values (`http`, `registry`, `upload`, `blank`, `pvc`), and `secret`/`cert_configmap` is set. This happens because the code indexes `source_spec[self.source]` even when that key was never initialized.

### Issue Context
This is in the deprecated `source` path (when `source_dict` is not provided). The failure is deterministic when `secret` or `cert_configmap` is non-`None` and `source` is unsupported.

### Fix Focus Areas
- ocp_resources/datavolume.py[204-217]

### Suggested fix
- Add explicit validation for `self.source` in the deprecated branch and raise a clear `ValueError` for unsupported values, **before** attempting to add `secretRef`/`certConfigMap`.
- Alternatively (less strict), ensure `source_spec.setdefault(self.source, {})` is executed before writing `secretRef`/`certConfigMap`, while still handling required per-source fields (like `url` for `http/registry`) appropriately.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Any types in DataVolume.__init__ 📘 Rule violation ⚙ Maintainability

Description

DataVolume.__init__ introduces multiple Any-typed parameters (source_dict, source_ref,
checkpoints, **kwargs) without documented justification, reducing type safety and defeating the
purpose of type hints. This violates the requirement to use concrete types for new/modified
signatures in optionally-typed languages.

Code

ocp_resources/datavolume.py[R81-101]

+        source_dict: dict[str, Any] | None = None,
+        size: str | None = None,
+        storage_class: str | None = None,
+        url: str | None = None,
+        content_type: str | None = None,
+        access_modes: str | None = None,
+        volume_mode: str | None = None,
+        cert_configmap: str | None = None,
+        secret: Secret | None = None,
+        hostpath_node: str | None = None,
+        source_pvc: str | None = None,
+        source_namespace: str | None = None,
+        source_ref: dict[str, Any] | None = None,
+        multus_annotation: str | None = None,
+        bind_immediate_annotation: bool | None = None,
+        preallocation: bool | None = None,
+        api_name: str | None = "pvc",
+        checkpoints: list[Any] | None = None,
+        final_checkpoint: bool | None = None,
+        priority_class_name: str | None = None,
+        **kwargs: Any,

Relevance

⭐⭐ Medium

Team often improves typing, but Any is common for schema-driven dicts/kwargs; no clear precedent to
remove here.

PR-#2437

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The checklist forbids introducing generic Any types in new/modified function signatures unless
there is an intentional, documented need. The updated DataVolume.__init__ signature includes
dict[str, Any], list[Any], and **kwargs: Any.

Rule 1834714: Require type hints on all new functions, methods, and parameters
ocp_resources/datavolume.py[80-102]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`DataVolume.__init__` uses `Any` in several newly/modified parameters (`source_dict`, `source_ref`, `checkpoints`, `**kwargs`) without a documented need.

## Issue Context
Compliance requires explicit type hints with concrete types (avoid `Any`/`object` unless intentionally required and documented).

## Fix Focus Areas
- ocp_resources/datavolume.py[80-102]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. PVC clone name None 🐞 Bug ≡ Correctness

Description

When using deprecated source="pvc", to_dict() writes "name": self.source_pvc without a default
or validation. Since source_pvc defaults to None, this can generate an invalid spec.source.pvc
with name: None and fail resource creation.

Code

ocp_resources/datavolume.py[R208-212]

+                elif self.source == "pvc":
+                    source_spec[self.source] = {
+                        "name": self.source_pvc,
+                        "namespace": self.source_namespace or self.namespace,
+                    }

Relevance

⭐⭐ Medium

Would add validation/default for required PVC name; team sometimes rejects stricter validation
changes without strong need.

PR-#2529

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The constructor defaults source_pvc to None, and the source=="pvc" branch writes it directly
into the manifest as the PVC clone source name.

ocp_resources/datavolume.py[78-102]
ocp_resources/datavolume.py[208-212]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
In the deprecated `source="pvc"` serialization path, the code sets `spec.source.pvc.name` directly from `self.source_pvc`. Because `source_pvc` defaults to `None`, callers that omit it will produce a manifest containing `name: null`.

### Issue Context
This occurs only when `source_dict` is not provided and `source` is used (deprecated). The generated manifest becomes invalid for PVC cloning.

### Fix Focus Areas
- ocp_resources/datavolume.py[208-212]

### Suggested fix
Choose one of:
- Validate: if `self.source == "pvc"` and `not self.source_pvc`, raise a `MissingRequiredArgumentError`/`ValueError` with a clear message.
- Backward compatible default: restore a safe fallback (e.g., previous behavior of defaulting to a non-empty name) if that is still desired for compatibility.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. DataVolume.__init__ docstring non-Google 📘 Rule violation ⚙ Maintainability

Description

The updated Args: entries use a non-Google format like param (type, default: ...) instead of
param: description, which violates the required Google-style docstring convention. This makes
docstrings inconsistent and harder to parse by standard tooling.

Code

ocp_resources/datavolume.py[R107-129]

+            source (str, default: None): source of DV - upload/http/pvc/registry/blank.
+            source_dict (dict[str, Any], default: None): DataVolume.source dictionary.
+            size (str, default: None): DataVolume size - format size+size unit, for example: "5Gi".
            storage_class (str, default: None): storage class name for DataVolume.
            url (str, default: None): url for importing DV, when source is http/registry.
-            content_type (str, default: "kubevirt"): DataVolume content type.
-            access_modes (str, default: None): DataVolume access mode.
+            content_type (str, default: None): DataVolume content type (e.g., "kubevirt", "archive").
+            access_modes (str, default: None): DataVolume access mode (e.g., "ReadWriteOnce", "ReadWriteMany").
+            volume_mode (str, default: None): DataVolume volume mode (e.g., "Filesystem", "Block").
            cert_configmap (str, default: None): name of config map for TLS certificates.
            secret (Secret, default: None): to be set as secretRef.
-            client (DynamicClient): DynamicClient to use.
-            volume_mode (str, default: None): DataVolume volume mode.
            hostpath_node (str, default: None): Node name to provision the DV on.
            source_pvc (str, default: None): PVC name for when cloning the DV.
            source_namespace (str, default: None): PVC namespace for when cloning the DV.
+            source_ref (dict[str, Any], default: None): SourceRef is an indirect reference to the source of data for the
+              requested DataVolume. Currently only "DataSource" is supported. Fields: kind (str), name (str), namespace (str)
            multus_annotation (str, default: None): network nad name.
-            bind_immediate_annotation (bool, default: None): when WaitForFirstConsumer is set in  StorageClass and DV
-            should be bound immediately.
+            bind_immediate_annotation (bool, default: None): when WaitForFirstConsumer is set in StorageClass and DV
+                should be bound immediately.
            preallocation (bool, default: None): preallocate disk space.
-            teardown (bool, default: True): Indicates if this resource would need to be deleted.
-            yaml_file (yaml, default: None): yaml file for the resource.
-            delete_timeout (int, default: 4 minutes): timeout associated with delete action.
-            api_name (str, default: "pvc"): api used for DV, pvc/storage
-            delete_after_completion (str, default: None): annotation for garbage collector - "true"/"false"
+            api_name (str, default: "pvc"): api used for DV (e.g., "storage", "pvc").
+            checkpoints (list[Any], default: None): list of DataVolumeCheckpoints for snapshot operations.
+            final_checkpoint (bool, default: None): indicates whether the current DataVolumeCheckpoint is the final one.
+            priority_class_name (str, default: None): priority class name for the DataVolume pod.

Relevance

⭐ Low

Similar request to switch Args to Google-style name: desc was explicitly rejected before.

PR-#2765

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The compliance rule requires Google-style docstrings; in Google style, parameters are documented as
name: description under Args:. The modified docstring lines document parameters using `name
(type, default: ...)`, which is a different style.

Rule 1834792: Inline docstrings must use Google-style triple-quoted blocks
ocp_resources/datavolume.py[103-130]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`DataVolume.__init__` docstring `Args:` section is not in Google style (uses `name (type, default: ...)` instead of `name: description`).

## Issue Context
Compliance requires Google-style triple-quoted docstrings with `Args:` entries formatted as `name: description`.

## Fix Focus Areas
- ocp_resources/datavolume.py[103-130]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. DataVolume.to_dict() returns None 📘 Rule violation ⚙ Maintainability

Description

The modified DataVolume.to_dict() performs serialization by mutating self.res but does not
return a dictionary, contradicting the requirement that resource classes provide `to_dict(self) ->
dict. This can break callers expecting to_dict()` to produce and return a serializable dict.

Code

ocp_resources/datavolume.py[R157-221]

+            self.res["spec"] = {}
+            _spec = self.res["spec"]
+
+            if self.checkpoints is not None:
+                _spec["checkpoints"] = self.checkpoints
+
+            if self.content_type is not None:
+                _spec["contentType"] = self.content_type
+
+            if self.final_checkpoint is not None:
+                _spec["finalCheckpoint"] = self.final_checkpoint
+
+            if self.preallocation is not None:
+                _spec["preallocation"] = self.preallocation
+
+            if self.priority_class_name is not None:
+                _spec["priorityClassName"] = self.priority_class_name
+
+            # Set api_name spec fields (pvc/storage)
+            if self.api_name is not None:
+                _spec[self.api_name] = {}
+
+                if self.access_modes is not None:
+                    _spec[self.api_name]["accessModes"] = [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}}
+
+            # Handle source configuration
+            if self.source_dict is not None:
+                _spec["source"] = self.source_dict
+            elif self.source is not None:
+                warn(
+                    "source is deprecated and will be removed in the next version. Use source_dict instead.",
+                    DeprecationWarning,
+                    stacklevel=2,
+                )
+
+                _spec["source"] = {}
+                source_spec = _spec["source"]
+
+                if self.source in ["http", "registry"]:
+                    source_spec[self.source] = {"url": self.url}
+                elif self.source in ["upload", "blank"]:
+                    source_spec[self.source] = {}
+                elif self.source == "pvc":
+                    source_spec[self.source] = {
+                        "name": self.source_pvc,
+                        "namespace": self.source_namespace or self.namespace,
+                    }
+
+                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
+
+            if self.source_ref is not None:
+                _spec["sourceRef"] = self.source_ref
+

Relevance

⭐ Low

Prior review rejected changing resource to_dict() from returning None to returning a dict.

PR-#2765

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The checklist requires resource classes to implement to_dict(self) -> dict and return a
dictionary. The DataVolume.to_dict() implementation is annotated as returning None and contains
no return statement despite constructing self.res["spec"] content.

Rule 1834979: Resource classes must define init and to_dict methods
ocp_resources/datavolume.py[154-235]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`DataVolume.to_dict()` is required to return a dictionary (`to_dict(self) -> dict`), but the current implementation returns `None` (no return statement) while mutating `self.res`.

## Issue Context
Compliance requires resource classes to define `to_dict` that returns a dictionary.

## Fix Focus Areas
- ocp_resources/datavolume.py[154-235]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---