If size_is_minsize is true, consider the size in sync if already larg… #125
Conversation
| @@ -35,6 +35,27 @@ | |||
| raise ArgumentError , "#{value} is not a valid logical volume size" | |||
| end | |||
| end | |||
| def insync?(is) | |||
| lvm_size_units = { "K" => 1, "M" => 1024, "G" => 1048576, "T" => 1073741824, "P" => 1099511627776, "E" => 1125899906842624 } | |||
There was a problem hiding this comment.
By convention all code is two spaces indents.
For clarity, please use 1024*1024, etc, instead of 1048576.
There was a problem hiding this comment.
I fixed the spacing, but I opted for 1024**2 etc. instead.
|
Congrats on getting all the tests green on your first try! The implementation looks fine in principle, I just nitpicked a few style issues. Please take care to use |
dfairhurst
force-pushed
the
feature/insync_size
branch
from
f9765ea
to
9bbd292
Compare
…er than desired size
|
Updated as requested. One thing that I could possibly change is to use \d instead of what was existing [0-9]. |
| current_size_bytes = $1.to_f | ||
| current_size_unit = $2.upcase | ||
| current_size_unit = $3.upcase |
There was a problem hiding this comment.
When tracking the unit handling code, I found another doozey in the overall code structure. Looking at https://github.com/puppetlabs/puppetlabs-lvm/pull/125/files#diff-d8b38c124d9da799f07a02cb607ba2deR125 , it uses lvs to convert the units, as lvm uses lowercase to denote multiples of 1024 and uppercase to denote multiples of 1000. o.O
See also http://linux.die.net/man/8/lvs --units.
There was a problem hiding this comment.
Is it really an issue if all the lvm commands called are passed with a lowercase unit, or just as bytes?
I don't think the module should have to support uppercase meaning multiples of 1000 instead of 1024. It should always assume 1024 as long as it calls the correct lvm commands (i.e. don't use uppercase units when calling lvm)
There was a problem hiding this comment.
This code - and that is not your fault! - makes my brain hurt. Indeed in L122, incoming units are always downcased.
There was a problem hiding this comment.
:) anything else remaining or OK for merging?
If size_is_minsize is true, consider the size in sync if already larg…
|
Thanks for your contribution! |
|
Thanks for the review + merge |
…er than desired size
Proposed fix for:
https://tickets.puppetlabs.com/browse/MODULES-2662