Handling of YAST_REUSE_LVM linuxrc option #1365
Conversation
|
Review was sent when I was still working on the comment (I clicked the wrong button, I guess). I was saying that I like the idea of That being said, I have doubts about adding the setting to the control file. I'm not saying is a bad idea, I literally mean I have doubts whether is a good idea. On the bright side, we will be prepared for configuring the default value per-product in the future without code changes. On the other hand, adding things to the control file definition is a live-lasting contract signed in blood. 😉 |
I had an idea that we can easily switch default behavior later on if we want to. Question is if it pays of to do it in a creepy way or we should hope in big bang. Adding a feature into control file even changing default proposal, both is radical. Which way brings us more benefits? I'm not sure ... but with control file it at least won't require our assistance in that decision (hope). |
src/lib/y2storage/storage_env.rb
Outdated
| # | ||
| # @return [Boolean, nil] boolean as explicitly set by user, nil if user set nothing | ||
| def requested_lvm_reuse | ||
| active?(ENV_REUSE_LVM, default: nil) |
There was a problem hiding this comment.
Sorry to be picky about this, but the problem already mentioned about boolean and nil is here just moved one level down. :-)
Still, there is a method active? that returns something that is not a boolean. You are actually deviating from the original meaning of that method which explicitly declares both the return value and the parameter default to be booleans.
If you want to introduce support for nil values, please do it at requested_lvm_reuse, but don't twist active?.
There was a problem hiding this comment.
ok, i tried and it didn't pass ... it happens ;-)
So another try is here.
There was a problem hiding this comment.
In general it's good. But I really miss unit tests.
We have a mock_env helper that makes it easy to test how things behave depending on the presence / absence of some boot parameters.
https://github.com/search?q=repo%3Ayast%2Fyast-storage-ng%20mock_env&type=code
src/lib/y2storage/storage_env.rb
Outdated
|
|
||
| return env_str_to_bool(value) if value | ||
|
|
||
| nil |
There was a problem hiding this comment.
Nitpicking comment: I think this would do the same and it's nicer to my Ruby eyes
def requested_lvm_reuse
value = read(ENV_REUSE_LVM)
return unless value
env_str_to_bool(value)
endHow could you easily check if both implementations are equivalent? Well, I you would have unit tests... 😉
|
✔️ Internal Jenkins job #5 successfully finished |
Problem
https://jira.suse.com/browse/PED-6407
The Storage Proposal in SLE 15 allows for creating a layout based on LVM. For historical reasons, it tries to reuse the existing LVM volume group for new installations. This sometimes produces a non-optimal, non-logical or even confusing disk setup.
Solution
linuxrc option to disable lvm reuse
Testing