-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes for bugs found during testing of 'missing parameters' changeset #64
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the commits except for the one with the extra comments look good to me.
library/blivet.py
Outdated
def _look_up_device(self): | ||
super(BlivetDiskVolume, self)._look_up_device() | ||
if not self._get_device_id(): | ||
# FAIL: no disks specified for volume |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment can be removed, it contains the same text as the exception in the next line
library/blivet.py
Outdated
raise BlivetAnsibleError("volume disks must be specified as a list") | ||
|
||
if self._device is None: | ||
# FAIL: failed to find the disk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment can be removed, it contains the same text as the exception in the next line
library/blivet.py
Outdated
super(BlivetDiskVolume, self)._look_up_device() | ||
if not self._get_device_id(): | ||
# FAIL: no disks specified for volume | ||
raise BlivetAnsibleError("no disks specified for volume '%s'" % self._volume['name']) # sure about this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment at the end of this line is irritating. Is there an issue that should be tracked or can it be removed?
52b8d4f
to
d6a8b5b
Compare
@dwlehman this is ready to be merged, right? |
Do you please have tests for those problems? |
I suppose the test disabled in 136e41d can be one of them |
maybe this : #43 (comment) is another instance? |
@@ -209,6 +209,8 @@ def _resize(self): | |||
raise BlivetAnsibleError("volume '%s' cannot be resized from %s to %s: %s" % (self._device.name, | |||
self._device.size, | |||
size, str(e))) | |||
elif size and self._device.size != size and not self._device.resizable: | |||
raise BlivetAnsibleError("volume '%s' cannot be resized from %s to %s" % (self._device.name, self._device.size, size)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intended that the role fails in this case already in the "get required packages" task, before the actual resizing attempt?
merging - but tests for bugs that it fixes are missing and will need to be added separately. |
I reverted master, this change breaks LUKS. |
NOTE: This will need to be rebased once the
safe_mode
changes are merged.Fix key for partition pool...
This leads to a failure (crash?) any time a pool of type 'partition' is present.
Don't unmount anything if only...
This one is pretty self-explanatory.
Add proper checking of specified disk...
We already validate the supplied value of
disks
for all pools, but not for disk volumes.Fix passing of fs create options...
We were incorrectly passing user-specified mkfs arguments to blivet, causing them to be ignored.
Fail on request to resize unresizable...
We should not silently fail if the user requests a resize of a non-resizable volume.
Remove partitions etc as needed...
This is sort of a special-case for disk volumes, but it could be needed for other types as well.
Fail w/ error if mount specified...
Error out early if the user specifies mounting of a non-mountable formatting type.
Add a default empty volume list to...
This solves current crashes when a pool is defined with no
volumes
list.