Accept path parameter in LogicalVolume initialization

[movitto]

No description provided.

[coveralls]

Coverage increased (+0.02%) when pulling 87d4c87 on movitto:lv-path into e00e317 on ManageIQ:master.

[jrafanie]

I think this is getting closer but the DEFAULT_PATH should really be defined by the volume group name. Please correct me if I'm wrong... cc/ @brandondunne

If @vg is 'volume_group'

1. LV.create 'lv', @vg, 256.gigabytes
   lv.name => 'lv'
   lv.path => '/dev/volume_group/lv'

2. LV.create '/dev/volume_group/lv', @vg, 256.gigabytes
   lv.name => 'lv',
   lv.path => '/dev/volume_group/lv'

If @vg is 'vg'

1. LV.create 'lv', @vg, 256.gigabytes
   lv.name => 'lv'
   lv.path => '/dev/vg/lv'

2. LV.create '/dev/vg/lv', @vg, 256.gigabytes
   lv.name => 'lv'
   lv.path => '/dev/vg/lv'

[movitto]

@jrafanie updated

[coveralls]

Coverage increased (+0.02%) when pulling 09d2055 on movitto:lv-path into e00e317 on ManageIQ:master.

[jrafanie]

Sorry @movitto, I wasn't clear in my description. Is it reasonable to create a LogicalVolume without a @volume_group? I don't think DEFAULT_PATH is needed. I think this else condition is not needed. We should always use the DEVICE_PATH and @volume_group.name to build the build.

[coveralls]

Coverage remained the same when pulling adbb466 on movitto:lv-path into d445353 on ManageIQ:master.

[movitto]

@jrafanie updated

[jrafanie]

@movitto Looks good other than what I put above... Although I don't know that I like my example that much. I"m sure you can make it better than mine ;-)

[Fryguy]

Use parens around method calls.

[coveralls]

Coverage increased (+0.02%) when pulling 3ef2d75 on movitto:lv-path into 9d8747a on ManageIQ:master.

[movitto]

@Fryguy updated

[jrafanie]

@movitto I have another idea.

You see how you have to check whether a path or name was provided here and also at line 82 below? I think all of this complexity can go away and you can just do LogicalVolume.new :name => name_or_path_value... so the .create method below should be unchanged, I think.

How about something like this:

def initialize(args = {})
 @volume_group = args[:volume_group]
 @sectors      = args[:sectors]
 self.path     = args[:name] # pass whatever was provided name or path, let the setter method handle it
 self.name     = args[:name]
 ...

end

def path=(value)
  @path = value.include?(File::SEPARATOR) ? value : DEVICE_PATH.join(@volume_group.name, value)
end

def name=(value)
  @name = value.include?(File::SEPARATOR) ? value.split(File::SEPARATOR).last : value
end

[movitto]

@jrafanie good idea, updated

[coveralls]

Coverage remained the same when pulling 887f46a on movitto:lv-path into 614e93d on ManageIQ:master.

[jrafanie]

Maybe id isn't the right variable name. I think of an numeric value when I think of id. Maybe name (the original) is best. Name can be the name of the lv or the path of the lv.

[jrafanie]

I think this is ready to merge once updated based on my last comments.

[coveralls]

Coverage remained the same when pulling ab74f62 on movitto:lv-path into 614e93d on ManageIQ:master.

[jrafanie]

Other than style differences, it looks good. 👍