support full disk devices as lvm pv in autoyast profile (bsc#1107298) #739
Conversation
wfeldt
commented
Sep 13, 2018
•
wfeldt
commented
- unit tests missing
| @@ -45,6 +45,7 @@ def self.attributes | |||
| { name: :keep_unknown_lv }, | |||
| { name: :lvm2 }, | |||
| { name: :is_lvm_vg }, | |||
| { name: :lvm_group }, | |||
There was a problem hiding this comment.
According to AutoYaST documentation, there is no lvm_group attribute in a <drive> section. Only <partition> sections have that.
There was a problem hiding this comment.
I know. But keeping that partition 0 hack is not really a good idea either; so I added the element also here.
There was a problem hiding this comment.
BTW, pesize is in drive and not in partition where it would logically belong to.
There was a problem hiding this comment.
The autoyast profile looks not very consistent here and we should try to make it more logical going forward.
There was a problem hiding this comment.
If it would be only lvm_group and pesize... There are quite some attributes in the current specification that are UTTERLY misplaced.
But in my opinion if we want to introduce another way of specifying the usage of full disks, we must first:
- decide/design how it's going to look like
- implement it
- then, document it
And that includes thinking about import, export, etc. and thinking about full disks as PV, full formatted disks, etc.
Just adding another attribute to the <drive> section "since we are here" sounds like the recipe to repeat the problem in the future (discovering via bug report than the code supported yet another way of doing things because it sounded like a good idea to whoever was touching the code). 😉
|
|
||
| # if there are no partitions use the entire device | ||
| if drive.partitions.empty? | ||
| result = planned_for_full_disk(drive) |
There was a problem hiding this comment.
This does the same in the case in which we have just one partition with partition_nr=0, create=false (which was the scope of the PBI) and if we have an empty list of partitions (which is a different scenario that is reflected in the current AutoYaST documentation).
There was a problem hiding this comment.
The created layout is the same as before in this case.
|
IMO, this PR in its current shape goes a little bit too much beyond duty. I would focus on supporting the current format (just one partition with Now that we are adding support to the partitioner to create more flexible layouts, we will need to find a way to specify all that in AutoYaST. But that's out of the scope of this PR/PBI and it's not something to be done just by thinking locally. |
This allows partition 0 to stand as an alias for the full disk.
|
replaced by #740 |
