Conversation
format = kv.DEFAULT_ALLOCATION_FORMAT | ||
else: | ||
format = str(opts[kv.DISK_ALLOCATION_FORMAT]) | ||
logging.debug("SETTING VMDK DISK ALLOCATION FORMAT to %s for %s", format, vmdk_path) |
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.
Don't use ALL CAPS. Use sentence case.
b7a4ce5
to
b0fb95c
Compare
@@ -75,7 +75,7 @@ | |||
OBJ_TOOL_CMD = "/usr/lib/vmware/osfs/bin/objtool open -u " | |||
OSFS_MKDIR_CMD = "/usr/lib/vmware/osfs/bin/osfs-mkdir -n " | |||
MKFS_CMD = BIN_LOC + "/mkfs.ext4 -qF -L " | |||
VMDK_CREATE_CMD = "/sbin/vmkfstools -d thin -c " | |||
VMDK_CREATE_CMD = "/sbin/vmkfstools" |
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.
nit: would be better to have VMDK_CREATE_CMD = "/sbin/vmkfstools -c"
not a big deal since the string needs to be formatted anyways, just makes it more symmetric with '-U' option
LGTM contingent on addressing I agree with Prashant's request to add valid choices to error message. Also a couple of nits - please take a look |
b0fb95c
to
8893a5f
Compare
Ensure format is valid. | ||
""" | ||
if not format.lower() in kv.VALID_ALLOCATION_FORMATS : | ||
raise ValidationError('Disk Allocation Format {0} does not exist. Valid options are: (thin(default), eagerzeroedthick, zeroedthick)'.format(format)) |
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.
I think this statement is way beyond 80char limit or whatever PEP8 recommends. You may want split & indent in 2 lines.
8893a5f
to
d72b1dd
Compare
I have made changes to the code according to the comments posted here. |
Fixes #427
Added option for user to select disk allocation format. Example:
format choices:
"zeroedthick", "thin" (default), "eagerzeroedthick"
Tested:
make all
and CI