Skip to content

Conversation

meinaLi
Copy link
Contributor

@meinaLi meinaLi commented Sep 16, 2025

In script a wrong function name write_datas() has been used. The correct one is write_data().

Summary by CodeRabbit

  • Tests
    • Corrected data-writing logic in the incremental backup (pull mode over TLS) test path, ensuring the full-backup flow with secret-based authentication writes guest disk data correctly.
    • Improves test accuracy and stability without altering runtime behavior or public interfaces.
    • No user-facing changes; impacts only test reliability and coverage.

In script a wrong function name write_datas() has been used. The correct
one is write_data().

Signed-off-by: meinaLi <[email protected]>
Copy link

coderabbitai bot commented Sep 16, 2025

Walkthrough

A single test file in incremental backup pull mode over TLS updates a function call inside pull_full_backup_to_file: it now invokes write_data() instead of write_datas() within the with_secret_uuid branch. No other logic, control flow, or public API/signature changes are introduced.

Changes

Cohort / File(s) Summary of Changes
Incremental backup TLS test
libvirt/tests/src/incremental_backup/incremental_backup_pull_mode_tls.py
Replaced call to write_datas() with write_data() in the with_secret_uuid code path; write_data() exists in the same file. No other modifications.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~6 minutes

Poem

I nudge my whiskers at the stream of bytes,
One call corrected, snug and right.
From datas to data—tidy delight—
Backups pull true through TLS night.
I thump approval, ears held high,
Small hop, clean path, tests comply.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "Inc_backup: minor fix to correct function name" succinctly and accurately describes the PR’s primary change — correcting an incorrect function call (write_datas() → write_data()) in the incremental backup test. It is a single, clear sentence that highlights the most important change without extraneous detail and matches the PR description and diff. This makes it understandable for a reviewer scanning history.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
libvirt/tests/src/incremental_backup/incremental_backup_pull_mode_tls.py (1)

160-164: Initialize cleanup variables to prevent UnboundLocalError on exceptions

If an error occurs before backup_file_path or secret_uuid are assigned (with with_secret_uuid true), the finally block can raise UnboundLocalError.

Apply this diff to safely initialize:

 qemu_config = LibvirtQemuConfig()
 disk_obj = disk_base.DiskBase(test, vm, params)
 tls_obj = None
+backup_file_path = None
+secret_uuid = None
 
 try:
🧹 Nitpick comments (2)
libvirt/tests/src/incremental_backup/incremental_backup_pull_mode_tls.py (2)

165-167: Unpack prepare_tls_env() return to avoid tuple in tls_obj

prepare_tls_env() returns (tls_obj, qemu_config). Assigning to tls_obj alone stores a tuple and is confusing for lifecycle handling.

Apply this diff:

-        tls_obj = prepare_tls_env(test, params, qemu_config)
+        tls_obj, qemu_config = prepare_tls_env(test, params, qemu_config)

203-205: Guard secret cleanup on presence of secret_uuid

Avoid calling secret_undefine with a possibly unassigned/None UUID.

Apply this diff:

-        if with_secret_uuid:
-            virsh.secret_undefine(secret_uuid, ignore_status=True)
-            if backup_file_path:
-                os.remove(backup_file_path)
+        if with_secret_uuid:
+            if secret_uuid:
+                virsh.secret_undefine(secret_uuid, ignore_status=True)
+            if backup_file_path:
+                os.remove(backup_file_path)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ef6dc96 and 4ff722c.

📒 Files selected for processing (1)
  • libvirt/tests/src/incremental_backup/incremental_backup_pull_mode_tls.py (1 hunks)
⏰ 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). (4)
  • GitHub Check: Python 3.12
  • GitHub Check: Python 3.11
  • GitHub Check: Python 3.9
  • GitHub Check: Python 3.8
🔇 Additional comments (1)
libvirt/tests/src/incremental_backup/incremental_backup_pull_mode_tls.py (1)

171-174: write_data() fix — approved

Search shows all remaining uses of write_datas are in libvirt/tests/src/incremental_backup/incremental_backup_pull_mode_info.py (definition and calls); libvirt/tests/src/incremental_backup/incremental_backup_pull_mode_tls.py defines def write_data() at line 112. No stale references remain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant