Merge pull request #544 from yast/ay-handle-proposal-settings

Handle proposal settings properly

package/autoyast2.changes

@@ -1,3 +1,10 @@
+-------------------------------------------------------------------
+Thu Nov 14 07:43:39 UTC 2019 - Imobach Gonzalez Sosa <igonzalezsosa@suse.com>
+
+- Do not override all storage proposal settings when importing
+  values from the profile (boo#1156539).
+- 4.2.21
+
 -------------------------------------------------------------------
 Tue Nov 12 15:49:47 UTC 2019 - Josef Reidinger <jreidinger@suse.com>
 

package/autoyast2.spec

@@ -22,7 +22,7 @@
 %endif
 
 Name:           autoyast2
-Version:        4.2.20
+Version:        4.2.21
 Release:        0
 Summary:        YaST2 - Automated Installation
 License:        GPL-2.0-only
@@ -54,8 +54,8 @@ BuildRequires:  yast2-slp
 BuildRequires:  yast2-country
 # Required for test suite testing one time sync
 BuildRequires:       yast2-ntp-client >= 4.0.1
-# AutoinstIssues::NoProposal
-BuildRequires:  yast2-storage-ng >= 4.0.160
+# Y2Storage::AutoinstProposal constructor receives a ProposalSettings object
+BuildRequires:  yast2-storage-ng >= 4.2.55
 # %%{_unitdir} macro definition is in a separate package since 13.1
 %if 0%{?suse_version} >= 1310
 BuildRequires:  systemd-rpm-macros
@@ -76,8 +76,8 @@ Requires:       yast2-network >= 3.1.145
 Requires:       yast2-schema >= 4.0.6
 Requires:       yast2-transfer >= 2.21.0
 Requires:       yast2-xml
-# AutoinstIssues::NoProposal
-Requires:       yast2-storage-ng >= 4.0.160
+# Y2Storage::AutoinstProposal constructor receives a ProposalSettings object
+Requires:       yast2-storage-ng >= 4.2.55
 Requires:       yast2-ruby-bindings >= 1.0.0
 
 Conflicts:      yast2-installation < 3.1.166

src/lib/autoinstall/storage_proposal.rb

@@ -38,11 +38,12 @@ class StorageProposal
     # Constructor
     #
     # @param partitioning [Array<Hash>] Partitioning section from the AutoYaST profile
+    # @param proposal_settings [Y2Storage::ProposalSettings] Guided proposal settings
     #
     # @see https://www.suse.com/documentation/sles-12/singlehtml/book_autoyast/book_autoyast.html#CreateProfile.Partitioning
-    def initialize(partitioning)
+    def initialize(partitioning, proposal_settings)
       @issues_list = Y2Storage::AutoinstIssues::List.new
-      build_proposal(partitioning)
+      build_proposal(partitioning, proposal_settings)
     end
 
     # Set the proposal on the StorageManager
@@ -84,11 +85,14 @@ def valid?
     #
     # * {Y2Storage::GuidedProposal} if {partitioning} is nil or empty;
     # * {Y2Storage::AutoinstProposal} in any other case.
-    def build_proposal(partitioning)
+    #
+    # @param partitioning [Array<Hash>] Partitioning section from the AutoYaST profile
+    # @param proposal_settings [Y2Storage::ProposalSettings] Guided proposal settings
+    def build_proposal(partitioning, proposal_settings)
       if partitioning.nil? || partitioning.empty?
-        @proposal = guided_proposal
+        @proposal = guided_proposal(proposal_settings)
       else
-        @proposal = autoinst_proposal(partitioning)
+        @proposal = autoinst_proposal(partitioning, proposal_settings)
         @proposal.propose
       end
       issues_list.add(:no_proposal) unless @proposal.devices
@@ -101,10 +105,13 @@ def build_proposal(partitioning)
     # @note A proposal is returned even when it is a failed one
     #
     # @param partitioning [Array<Hash>] Partitioning specification from AutoYaST profile
+    # @param proposal_settings [Y2Storage::ProposalSettings] Guided proposal settings
     # @return [Y2Storage::AutoinstProposal]
-    def autoinst_proposal(partitioning)
+    def autoinst_proposal(partitioning, proposal_settings)
       log.info "Creating an autoinstall proposal"
-      Y2Storage::AutoinstProposal.new(partitioning: partitioning, issues_list: issues_list)
+      Y2Storage::AutoinstProposal.new(
+        partitioning: partitioning, proposal_settings: proposal_settings, issues_list: issues_list
+      )
     end
 
     # Return a GuidedProposal according to product's proposal setting
@@ -114,11 +121,13 @@ def autoinst_proposal(partitioning)
     #
     # @see Y2Storage::GuidedProposal.initial
     #
+    #
+    # @param proposal_settings [Y2Storage::ProposalSettings] Guided proposal settings
     # @return [Y2Storage::GuidedProposal]
-    def guided_proposal
+    def guided_proposal(proposal_settings)
       log.info "Creating a guided proposal"
       # TODO: add specific issue when proposal fails because there are no devices
-      Y2Storage::GuidedProposal.initial
+      Y2Storage::GuidedProposal.initial(settings: proposal_settings)
     end
 
     # Handle Y2Storage exceptions

src/modules/AutoinstStorage.rb

@@ -17,6 +17,8 @@ class AutoinstStorageClass < Module
 
     # @return [Hash] General settings (from +storage/general+ profile section)
     attr_accessor :general_settings
+    # @return [Y2Storage::ProposalSettings] settings from +storage/general/proposal+
+    attr_reader :proposal_settings
 
     def main
       Yast.import "UI"
@@ -78,8 +80,7 @@ def import_general_settings(settings)
       self.general_settings = settings.clone
 
       # Override product settings from control file
-      control_settings = Yast::ProductFeatures.GetSection("partitioning") || {}
-      Yast::ProductFeatures.SetSection("partitioning", control_settings.merge(general_settings))
+      @proposal_settings = proposal_settings_from_profile(settings["proposal"])
     end
 
     # Import Fstab data
@@ -249,7 +250,7 @@ def Write
     # @param  partitioning [Array<Hash>] Profile settings (list of drives for custom partitioning)
     # @return [Boolean] success
     def build_proposal(partitioning)
-      proposal = Y2Autoinstallation::StorageProposal.new(partitioning)
+      proposal = Y2Autoinstallation::StorageProposal.new(partitioning, proposal_settings)
       if valid_proposal?(proposal)
         log.info "Saving successful proposal: #{proposal.inspect}"
         proposal.save
@@ -317,6 +318,43 @@ def preprocessed_settings(settings)
       preprocessor = Y2Autoinstallation::PartitioningPreprocessor.new
       preprocessor.run(settings)
     end
+
+    ALLOWED_OVERRIDES = [:lvm, :encryption_password].freeze
+    private_constant :ALLOWED_OVERRIDES
+    DELETE_RESIZE_OVERRIDES = [
+      :windows_delete_mode, :linux_delete_mode, :other_delete_mode, :resize_windows
+    ].freeze
+    private_constant :DELETE_RESIZE_OVERRIDES
+    # Returns the ProposalSettings having into account the values from the profile
+    #
+    # @param profile [Hash,nil] +general/storage/proposal+ section from the profile
+    # @return [Y2Storage::ProposalSettings]
+    def proposal_settings_from_profile(profile)
+      profile ||= {}
+      settings = Y2Storage::ProposalSettings.new_for_current_product
+
+      allowed_overrides = ALLOWED_OVERRIDES
+      allowed_overrides += DELETE_RESIZE_OVERRIDES if settings.delete_resize_configurable
+
+      ignored = []
+      profile.each do |key, value|
+        meth = "#{key}="
+        if settings.respond_to?(meth) && allowed_overrides.include?(key.to_sym)
+          settings.public_send(meth, value)
+        else
+          ignored << key
+        end
+      end
+
+      unless ignored.empty?
+        log.warn(
+          "Ignoring these elements from the general/storage/proposal " \
+            "profile section: #{ignored.join(", ")}"
+        )
+      end
+
+      settings
+    end
   end
 
   AutoinstStorage = AutoinstStorageClass.new

test/autoinst_storage_test.rb

@@ -58,7 +58,7 @@
     it "preprocess given settings" do
       expect(preprocessor).to receive(:run).with(ask_settings)
       expect(Y2Autoinstallation::StorageProposal).to receive(:new)
-        .with(settings)
+        .with(settings, anything)
         .and_return(storage_proposal)
       subject.Import([{ "device" => "ask" }])
     end
@@ -195,24 +195,92 @@
   end
 
   describe "#import_general_settings" do
-    let(:profile) { { "proposal_lvm" => true } }
-    let(:partitioning_features) { { "foo" => "bar" } }
+    let(:profile) do
+      { "proposal" => proposal }
+    end
+    let(:proposal) do
+      { "lvm"                 => true,
+        "windows_delete_mode" => :all,
+        "linux_delete_mode"   => :all,
+        "other_delete_mode"   => :all,
+        "resize_windows"      => true,
+        "encryption_password" => "12345678" }
+    end
+    let(:delete_resize_configurable) { true }
+    let(:proposal_settings) do
+      Y2Storage::ProposalSettings.new.tap do |settings|
+        settings.windows_delete_mode = :ondemand
+        settings.delete_resize_configurable = delete_resize_configurable
+      end
+    end
 
-    around do |example|
-      old_partitioning = Yast::ProductFeatures.GetSection("partitioning")
-      Yast::ProductFeatures.SetSection("partitioning", partitioning_features)
-      example.call
-      Yast::ProductFeatures.SetSection("partitioning", old_partitioning)
+    before do
+      allow(Y2Storage::ProposalSettings).to receive(:new_for_current_product)
+        .and_return(proposal_settings)
     end
 
-    it "overrides control file values" do
+    it "imports proposal settings from the profile" do
       subject.import_general_settings(profile)
-      expect(Yast::ProductFeatures.GetSection("partitioning")).to include("proposal_lvm" => true)
+      expect(subject.proposal_settings.lvm).to eq(true)
     end
 
-    it "keeps not overriden values" do
+    it "does not log any warning" do
+      expect(subject.log).to_not receive(:warn)
       subject.import_general_settings(profile)
-      expect(Yast::ProductFeatures.GetSection("partitioning")).to include("foo" => "bar")
+    end
+
+    context "when a value is not set in the profile" do
+      let(:proposal) do
+        { "lvm" => true }
+      end
+
+      it "keeps the product default" do
+        subject.import_general_settings(profile)
+        expect(subject.proposal_settings.windows_delete_mode).to eq(:ondemand)
+      end
+    end
+
+    context "when no value is set in the profile" do
+      let(:proposal) { nil }
+
+      it "keeps the product defaults" do
+        subject.import_general_settings(profile)
+        expect(subject.proposal_settings.windows_delete_mode).to eq(:ondemand)
+      end
+    end
+
+    context "when unknown proposal settings are specified" do
+      let(:proposal) { { "foo" => "bar" } }
+
+      it "logs a warning" do
+        expect(subject.log).to receive(:warn).with(/Ignoring.*foo/)
+        subject.import_general_settings(profile)
+      end
+    end
+
+    context "when the product does not allow to configure resizing or delete modes" do
+      let(:delete_resize_configurable) { false }
+      let(:proposal) do
+        {
+          "windows_delete_mode" => :all,
+          "linux_delete_mode"   => :all,
+          "other_delete_mode"   => :all,
+          "resize_windows"      => true
+        }
+      end
+
+      it "ignores related options" do
+        subject.import_general_settings(profile)
+        expect(subject.proposal_settings.windows_delete_mode).to eq(:ondemand)
+        expect(subject.proposal_settings.linux_delete_mode).to be_nil
+        expect(subject.proposal_settings.other_delete_mode).to be_nil
+        expect(subject.proposal_settings.resize_windows).to be_nil
+      end
+
+      it "logs a warning" do
+        expect(subject.log).to receive(:warn).with(/Ignoring.*windows_delete_mode/)
+        subject.import_general_settings(profile)
+      end
     end
   end
 end

test/lib/storage_proposal_test.rb

@@ -4,7 +4,7 @@
 require "autoinstall/storage_proposal"
 
 describe Y2Autoinstallation::StorageProposal do
-  subject(:storage_proposal) { described_class.new(profile) }
+  subject(:storage_proposal) { described_class.new(profile, proposal_settings) }
 
   let(:storage_manager) { double(Y2Storage::StorageManager) }
   let(:devicegraph) { instance_double(Y2Storage::Devicegraph) }
@@ -16,6 +16,7 @@
       devices: devicegraph)
   end
   let(:profile) { [{ "device" => "/dev/sda" }] }
+  let(:proposal_settings) { Y2Storage::ProposalSettings.new }
 
   before do
     allow(Y2Storage::StorageManager).to receive(:instance)
@@ -35,7 +36,6 @@
         expect(Y2Storage::GuidedProposal).to receive(:initial)
         expect(storage_proposal.proposal).to be(guided_proposal)
       end
-
     end
 
     context "when profile contains an empty set of partitions" do
@@ -52,7 +52,8 @@
 
       it "creates an autoinstall proposal" do
         expect(Y2Storage::AutoinstProposal).to receive(:new)
-          .with(partitioning: profile, issues_list: anything).and_return(autoinst_proposal)
+          .with(partitioning: profile, proposal_settings: proposal_settings, issues_list: anything)
+          .and_return(autoinst_proposal)
         expect(autoinst_proposal).to receive(:propose)
         expect(storage_proposal.proposal).to be(autoinst_proposal)
       end