fix: check for no disks specified and report correct error #575
Conversation
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
Cause: The code did not check for missing or empty disks in the right place. Consequence: The role would issue a strange error which did not indicate that the problem is that no disks were specified. Fix: Check for missing disks early and report appropriate error. Result: The role will report an appropriate error message if no disks are specified. Signed-off-by: Rich Megginson <rmeggins@redhat.com>
Reviewer's GuideAdds an early check in BlivetDiskVolume to handle volumes with no disks specified by returning None as the device id, and extends the Ansible test suite to cover the no-disks error case plus generated tests for multiple partition layouts on NVMe and SCSI. Class diagram for updated BlivetDiskVolume device id handlingclassDiagram
class BlivetVolume {
+dict _volume
+string name
+string id
+get_device_id()
+_type_check()
}
class BlivetDiskVolume {
+dict _volume
+string name
+string id
+_get_device_id() string
+_type_check()
}
BlivetVolume <|-- BlivetDiskVolume
BlivetVolume : _volume holds volume configuration
BlivetDiskVolume : blivet_device_class = devices.DiskDevice
BlivetDiskVolume : _get_device_id() checks _volume.disks
BlivetDiskVolume : if _volume.disks missing or empty returns None
BlivetDiskVolume : if _volume.disks nonempty returns first disk identifier
Flow diagram for BlivetDiskVolume _get_device_id no-disks error handlingflowchart TD
A[Start _get_device_id] --> B[Read disks from _volume using key disks with default empty list]
B --> C{Is disks empty?}
C -- Yes --> D[Return None as device id]
C -- No --> E[Select first element of disks]
E --> F[Return first disk as device id]
D --> G[Caller detects missing device id and reports no disks specified error]
F --> H[Caller continues with disk operations]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
BlivetDiskVolume._get_device_id, you build adiskslocal variable but then ignore it in the return; consider usingdisks[0]for consistency and to avoid duplicating the lookup logic. - Returning
Nonefrom_get_device_idto represent "no disks" relies on implicit behavior further up the stack; consider handling this case more explicitly (e.g., by raising a clear error or performing the validation at the call site) so the failure mode is easier to follow in the code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `BlivetDiskVolume._get_device_id`, you build a `disks` local variable but then ignore it in the return; consider using `disks[0]` for consistency and to avoid duplicating the lookup logic.
- Returning `None` from `_get_device_id` to represent "no disks" relies on implicit behavior further up the stack; consider handling this case more explicitly (e.g., by raising a clear error or performing the validation at the call site) so the failure mode is easier to follow in the code.
## Individual Comments
### Comment 1
<location> `library/blivet.py:943-944` </location>
<code_context>
blivet_device_class = devices.DiskDevice
def _get_device_id(self):
+ disks = self._volume.get('disks', [])
+ if not disks:
+ return None
return self._volume['disks'][0]
</code_context>
<issue_to_address>
**question (bug_risk):** Clarify the intended behavior change when `disks` is missing or empty (now returning `None`).
Previously this would raise a `KeyError`/`IndexError` when `disks` was missing or empty; now it returns `None`. Please confirm all callers either expect `None` as a valid "no device" value or explicitly handle it, rather than relying on the earlier exception behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+943
to
+944
| disks = self._volume.get('disks', []) | ||
| if not disks: |
There was a problem hiding this comment.
question (bug_risk): Clarify the intended behavior change when disks is missing or empty (now returning None).
Previously this would raise a KeyError/IndexError when disks was missing or empty; now it returns None. Please confirm all callers either expect None as a valid "no device" value or explicitly handle it, rather than relying on the earlier exception behavior.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #575 +/- ##
==========================================
- Coverage 16.54% 10.34% -6.21%
==========================================
Files 2 8 +6
Lines 284 2020 +1736
Branches 79 0 -79
==========================================
+ Hits 47 209 +162
- Misses 237 1811 +1574
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
[citest] |
vojtechtrefny
approved these changes
Dec 31, 2025
Contributor
Author
|
[citest] |
Contributor
Author
|
testing farm ci is broken :-( investigating |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Cause: The code did not check for missing or empty disks in the right place.
Consequence: The role would issue a strange error which did not indicate that the
problem is that no disks were specified.
Fix: Check for missing disks early and report appropriate error.
Result: The role will report an appropriate error message if no disks
are specified.
Signed-off-by: Rich Megginson rmeggins@redhat.com
Summary by Sourcery
Improve disk volume handling when no disks are specified and extend coverage with new storage tests.
Bug Fixes:
Tests: