Skip to content

Conversation

movitto
Copy link
Contributor

@movitto movitto commented Aug 29, 2013

No description provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling d42a52f on movitto:lvm-extents into a958afe on ManageIQ:master.

@jrafanie
Copy link
Member

@movitto I'll test it out and let you know.

@Fryguy
Copy link
Member

Fryguy commented Aug 29, 2013

I'm not big on having the API for the values for size or extents to be the strings that parted would expect. As a caller, I would expect to pass something like 256.gigabytes to size and for extents one of: 100, '100%', or 100.percent (although I'm not sure that third one's a valid method)

@jrafanie
Copy link
Member

Yeah, @movitto, after seeing @Fryguy's comment, I agree.

I like:

LogicalVolume.create 'lv', @vg, :extents => 100

and

LogicalVolume.create 'lv', @vg, :size => 256.gigabytes  # a Fixnum in bytes

It hides parted's implementation over something like this:

LogicalVolume.create 'lv', @vg, :extents => '100%FREE'
LogicalVolume.create 'lv', @vg, :size => '256G'

Both values should be variants of Fixnums and we can convert the extents to a percent string like:
100 => "100%FREE"

And size value to a parted size string:
256.gigabytes => "256G"

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.39%) when pulling f145268 on movitto:lvm-extents into a958afe on ManageIQ:master.

@movitto
Copy link
Contributor Author

movitto commented Aug 29, 2013

@jrafanie @Fryguy updated

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@movitto Does it make sense to simplify this conditional by assuming if the 3rd argument is > 100, the number is a size, otherwise it's a percent?

def self.create(name, vg, value)
  ...
  if value <= 100
    params.merge!({'-l' => "#{value}%FREE"})
  else
    params.merge!({'-L' => bytes_to_string(value)})
  end
  ...
end

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.39%) when pulling b5a6802 on movitto:lvm-extents into 920fcbf on ManageIQ:master.

@movitto
Copy link
Contributor Author

movitto commented Aug 29, 2013

@jrafanie sounds good to me, updated

jrafanie added a commit that referenced this pull request Aug 30, 2013
accept extents as option param in LogicalVolume#create
@jrafanie jrafanie merged commit 07a33b8 into ManageIQ:master Aug 30, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants