Fix: Partition unmount for issue 4463

[xianglongfei-8888]

#4463

I have modified the unmount () method of the Partition class, which now supports specifying a mount point during uninstallation. If no mount point parameter is provided, the mount point of the current object will be used. This allows for specifying mount points during class initialization and uninstallation to maintain consistency.

Summary by CodeRabbit

• New Features
  • The unmount operation now accepts an optional mountpoint parameter, allowing users to specify custom mount points during unmount operations for improved flexibility.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The unmount API in avocado/utils/partition.py is extended with an optional mountpoint parameter. The method signature changes from def unmount(self, force=True) to def unmount(self, force=True, mountpoint=None). The implementation is updated to only derive the mountpoint from the device when no mountpoint is explicitly provided, preserving existing behavior. The docstring is updated to document the new parameter and its behavior.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Review the updated method signature and verify backwards compatibility with the optional parameter
• Verify the conditional logic correctly determines mountpoint only when not explicitly provided
• Validate the docstring accurately documents the new parameter and expected behavior
• Confirm the early return logic when not mounted remains unchanged

Pre-merge checks

✅ 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 clearly and specifically references the main change: extending the Partition.unmount() method to accept an optional mountpoint parameter to address issue 4463.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

---

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 and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

avocado/utils/partition.py (1)

304-305: Clarify the docstring with usage guidance and safety notes.

The docstring documents the new parameter but could be clearer about when to use it and any safety considerations.

Consider enhancing the docstring:

-        :param force: Whether to force unmount if the standard unmount fails.
-        :param mountpoint: Optional mountpoint to unmount. If not provided, the current mountpoint of the partition object will be used.
+        :param force: Whether to force unmount if the standard unmount fails.
+        :param mountpoint: Optional mountpoint to unmount. If not provided, 
+                          the method will auto-detect the mountpoint from the device.
+                          Use this parameter when you need to specify a mountpoint 
+                          that differs from the partition object's stored value.
         :return: 1 on success, 2 on force umount success

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 74b7379 and 2ee03b2.

📒 Files selected for processing (1)

• avocado/utils/partition.py (2 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). (26)

• GitHub Check: rpm-build:fedora-41-ppc64le
• GitHub Check: rpm-build:epel-9-x86_64
• GitHub Check: rpm-build:fedora-42-x86_64
• GitHub Check: rpm-build:fedora-41-aarch64
• GitHub Check: rpm-build:fedora-rawhide-x86_64
• GitHub Check: rpm-build:fedora-43-x86_64
• GitHub Check: rpm-build:centos-stream-9-x86_64
• GitHub Check: rpm-build:fedora-41-s390x
• GitHub Check: rpm-build:fedora-41-aarch64
• GitHub Check: rpm-build:fedora-41-s390x
• GitHub Check: rpm-build:fedora-42-x86_64
• GitHub Check: rpm-build:centos-stream-9-x86_64
• GitHub Check: rpm-build:fedora-43-x86_64
• GitHub Check: rpm-build:fedora-41-ppc64le
• GitHub Check: rpm-build:fedora-41-x86_64
• GitHub Check: rpm-build:fedora-rawhide-x86_64
• GitHub Check: rpm-build:epel-9-x86_64
• GitHub Check: rpm-build:fedora-41-aarch64
• GitHub Check: rpm-build:centos-stream-9-x86_64
• GitHub Check: rpm-build:fedora-41-ppc64le
• GitHub Check: rpm-build:fedora-41-x86_64
• GitHub Check: rpm-build:epel-9-x86_64
• GitHub Check: rpm-build:fedora-rawhide-x86_64
• GitHub Check: rpm-build:fedora-43-x86_64
• GitHub Check: rpm-build:fedora-42-x86_64
• GitHub Check: rpm-build:fedora-41-s390x

🔇 Additional comments (1)

avocado/utils/partition.py (1)

310-311: Review comment is incorrect and should be disregarded.

The code is already correct and idempotent. The self.mountpoint attribute is a configured default (set once at construction), not a live state indicator. The get_mountpoint() method queries actual OS mount state from /proc/mounts, not self.mountpoint. After successful unmount, OS file systems are updated, so subsequent unmount() calls return early with no error. Clearing self.mountpoint would incorrectly break the mount() method's ability to use the stored default on subsequent operations.

Likely an incorrect or invalid review comment.

[clebergnu]

LGTM, the CI failure is unrelated.

[clebergnu]

This could be in an separate commit, but... it's probably me being too picky.