-
Notifications
You must be signed in to change notification settings - Fork 189
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.
Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @azzamsa)
pyvcloud/vcd/utils.py, line 552 at r1 (raw file):
result['id'] = extract_id(disk.get('id')) result['status'] = disk.get('status') result['size'] = humanfriendly.format_size(int(disk.get('sizeMb')))
This change is introduced in API version 33.0. For older versions, you still need to get it from 'size' attribute. Please add a check to read correct attribute depending on API version.
Also requesting to test it manually with both API version 33.0 and 32.0.
Thanks for the input. To get the api version, I need to do something like: size_key = 'size'
if client.get_api_version() < ApiVersion.VERSION_33.value:
size_key = 'sizeMb' because Any cleaner solution? |
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.
We can first check 'sizeMb' if it doesn't have a valid value, read 'size'. Since this is a mandatory parameter, either of the two will have a valid value. This way, we can skip checking API version.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @azzamsa)
Done. |
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.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @azzamsa and @shashim22)
pyvcloud/vcd/utils.py, line 552 at r2 (raw file):
result['id'] = extract_id(disk.get('id')) result['status'] = disk.get('status') if 'sizeMb' in disk.keys():
I think you missed to assign 'size' to size_key before the IF block. Also, in case when 'sizeMb' is set, you will have to multiple by 1024 to get the size in bytes.
the correct formula would be:
Tested and compared the |
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.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @azzamsa and @shashim22)
pyvcloud/vcd/utils.py, line 552 at r2 (raw file):
Previously, shashim22 wrote…
I think you missed to assign 'size' to size_key before the IF block. Also, in case when 'sizeMb' is set, you will have to multiple by 1024 to get the size in bytes.
Thanks for pointing it out, that was a typo. We should use 1024*1024 to be precise.
In the IF block, size_in_bytes should be converted to int:
size_in_bytes = int(disk.get('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.
https://pypi.org/project/humanfriendly/
humanfriendly.format_size(bytes, binary=True) converts the size accurately. If you don't provide binary=True, it uses 1000 in place of 1024.
Try to look at other methods in vdc.py, you can see that we have used 1024*1024 at multiple places.
vCloud director converts the same way. For example, if you give 2000 MB as disk size, it will show 1.86 GB.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @azzamsa and @shashim22)
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.
Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @azzamsa)
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @azzamsa)
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.
Please mark open issues as resolved so that I can merge the code.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @azzamsa)
pyvcloud/vcd/utils.py, line 552 at r2 (raw file): Previously, shashim22 wrote…
Done. |
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.
Reviewable status:
complete! all files reviewed, all discussions resolved
disk
object has nosize
attribute, butsizeMb
.Related: vmware-archive/vcd-cli#527
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)