Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handling of YAST_REUSE_LVM linuxrc option #1365

Merged
merged 11 commits into from
Feb 15, 2024
Merged

Handling of YAST_REUSE_LVM linuxrc option #1365

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8a1ef21
Handling of YAST_REUSE_LVM linuxrc option
mchf Jan 31, 2024
ebd05a5
Happy rubocop
mchf Feb 1, 2024
9fe9f4e
Relocated default setup of lvm reuse flag
mchf Feb 2, 2024
9f06c61
Cleanup
mchf Feb 2, 2024
66165b2
Minor adaption to ruby standards
mchf Feb 5, 2024
951ba96
Minor refactoring
mchf Feb 6, 2024
366e6f3
Do not introduce loading lvm_vg_reuse from control file
mchf Feb 7, 2024
1f4516e
Updated the testsuite to cover requested_lvm_reuse
mchf Feb 14, 2024
fd69273
Minor code reorganization
mchf Feb 14, 2024
6ebb66b
Minor testsuite update
mchf Feb 14, 2024
eb6aa7d
Updated changelog
mchf Feb 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/lib/y2storage/proposal/lvm_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
require "y2storage/disk_size"
require "y2storage/exceptions"
require "y2storage/planned/lvm_vg"
require "y2storage/storage_env"
mchf marked this conversation as resolved.
Show resolved Hide resolved

module Y2Storage
module Proposal
Expand Down Expand Up @@ -166,8 +167,6 @@ def new_volume_group
# @return [Boolean]
def try_to_reuse?
# Setting introduced to completely avoid LVM reusing in D-Installer.
# It's a new playground, so no need to carry YaST behaviors that have
# proven to be confusing.
return false if settings.lvm_vg_reuse == false

# We introduced this when we still didn't have a mechanism to activate
Expand Down
10 changes: 10 additions & 0 deletions src/lib/y2storage/proposal_settings.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,7 @@ def self.new_for_current_product
def for_current_product
apply_defaults
load_features
apply_user_enforced
end

# Produces a deep copy of settings
Expand Down Expand Up @@ -401,6 +402,14 @@ def apply_defaults
end
end

# Some values can be explicitly enforced by user
# This setting should have precendence over everything else
def apply_user_enforced
value = StorageEnv.instance.reuse_lvm?
mchf marked this conversation as resolved.
Show resolved Hide resolved

send(:lvm_vg_reuse, StorageEnv.instance.reuse_lvm?) if !value.nil?
mchf marked this conversation as resolved.
Show resolved Hide resolved
end

# Overrides the settings with values read from the YaST product features
# (i.e. values in /control.xml).
#
Expand All @@ -414,6 +423,7 @@ def load_features
load_feature(:proposal, :other_delete_mode)
load_feature(:proposal, :delete_resize_configurable)
load_feature(:proposal, :lvm_vg_strategy)
load_feature(:proposal, :lvm_vg_reuse)
mchf marked this conversation as resolved.
Show resolved Hide resolved
load_feature(:proposal, :allocate_volume_mode)
load_feature(:proposal, :multidisk_first)
load_size_feature(:proposal, :lvm_vg_size)
Expand Down
12 changes: 12 additions & 0 deletions src/lib/y2storage/storage_env.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,11 @@ class StorageEnv

ENV_LIBSTORAGE_IGNORE_PROBE_ERRORS = "LIBSTORAGE_IGNORE_PROBE_ERRORS".freeze

ENV_REUSE_LVM = "YAST_REUSE_LVM".freeze

private_constant :ENV_MULTIPATH, :ENV_BIOS_RAID, :ENV_ACTIVATE_LUKS, :ENV_LUKS2_AVAILABLE
private_constant :ENV_LIBSTORAGE_IGNORE_PROBE_ERRORS
private_constant :ENV_REUSE_LVM

def initialize
reset_cache
Expand Down Expand Up @@ -89,6 +92,15 @@ def luks2_available?
active?(ENV_LUKS2_AVAILABLE, default: false)
end

# Whether YaST should reuse existing LVM
#
# see jsc#PED-6407 or jsc#IBM-1315
#
# @return [Boolean, nil] boolean as explicitly set by user, nil if user set nothing
def reuse_lvm?
active?(ENV_REUSE_LVM, default: nil)
mchf marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

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?.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, i tried and it didn't pass ... it happens ;-)

So another try is here.

end

# Whether errors during libstorage probing should be ignored.
#
# See bsc#1177332:
Expand Down
Loading