Add released updates.

[varkoly]

Problem

A lot of changes was made in my fork which are already released. Now I want to push it upstream

[lslezak]

LGTM, just some ideas for improvements.

As this is for SP2 you can fix that only in master if you think it's too dangerous for a maintenance update.

[lslezak]

• Is the trailing space in the message OK?
• Quoting is inconsistent, most of the code uses single quotes (`) , here are double quotes (")
  (In YaST we use double quotes, but I'm fine with single quotes, it just should be consistent.)

[lslezak]

Just a note: do not forget that in Ruby ^ means "start of line", not "start of the string" as usually...

[lslezak]

There is a lot of duplication from the previous method, maybe it would make sense to share the common part. But as this is for a maintenance update it would be more safe to do this only in the master branch:

def adjust_non_production_system(system_id, options = {})
  log.info "--- called #{self.class}.#{__callee__}(#{system_id}, #{options.inspect}) ---"
  adjust_system(system_id, options, 'non-production')
end

def adjust_production_system(system_id, options = {})
  log.info "--- called #{self.class}.#{__callee__}(#{system_id}, #{options.inspect}) ---"
  adjust_system(system_id, options, 'production') do |ini|
    ini.set_config('system_replication', 'preload_column_tables', options[:preload_column_tables])
  end
end

def adjust_system(system_id, options = {}, system_type = "", &block)
  # Change the global.ini
  global_ini_path = HANA_GLOBAL_INI % system_id.upcase
  unless File.exist?(global_ini_path)
    NodeLogger.error "Could not adjust global.ini for the #{system_type} system:"
    NodeLogger.output "File #{global_ini_path} does not exist"
    return false
  end
  begin
    global_ini = CFA::GlobalIni.new(global_ini_path)
    global_ini.load
    global_ini.set_config('memorymanager', 'global_allocation_limit', options[:global_alloc_limit])
    global_ini.set_config('ha_dr_provider_SAPHanaSR', 'provider', 'SAPHanaSR')
    global_ini.set_config('ha_dr_provider_SAPHanaSR', 'path', '/usr/share/SAPHanaSR')
    global_ini.set_config('ha_dr_provider_SAPHanaSR', 'execution_order', '1')
    global_ini.set_config('trace', 'ha_dr_saphanasr', 'info')

    block.call(global_ini) if block_given?

    global_ini.save
  rescue StandardError => e
    NodeLogger.error "Could not adjust global.ini for the #{system_type} system:"
    NodeLogger.output e.message
    return false
  else
    NodeLogger.info "Successfully adjusted global.ini for the #{system_type} system #{system_id}"
    true
  end
end

You can make the adjust_system method private.

Untested! Quick Hack! Please test it well!

[varkoly]

I've applied your suggestions in master.

[lslezak]

wmods is unused, you can remove that

[lslezak]

Some notes:

• describe should be used for methods, if you need to group some tests use context. (Technically it is irrelevant, one of them is just an alias to the other one, but it makes the tests better structured and more readable.)
• describe should use a method name if possible/makes sense
• you can put the repeating parts into before block
• it should describe the action and the result

You can find more suggestions at https://www.betterspecs.org/

Proposal:

context 'SAP naming tests' do
  before do
    subject.silent = true
  end

  describe "#identifier" do
    it "returns true for valid IDs" do
      expect(subject.identifier('1Nuremberg')).to eq true
      expect(subject.identifier('1Nuremberg-A')).to eq true
      expect(subject.identifier('1Nuremberg_BA')).to eq true
      expect(subject.identifier('SuSE-1_2')).to eq true
    end

    it "returns false for invalid IDs" do
      expect(subject.identifier('Nürnberg')).to eq false
      expect(subject.identifier('-Nuremberg')).to eq false
      expect(subject.identifier('_Nuremberg')).to eq false
      expect(subject.identifier('*WalDorf-1_2/')).to eq false
      end
  end

  describe "#sap_sid" do
    it "returns true for valid IDs" do
      expect(subject.sap_sid('X12')).to eq true
    end

    it "returns false for invalid IDs" do
      expect(subject.sap_sid('123')).to eq false
      expect(subject.sap_sid('NOT')).to eq false
    end
  end

  describe "#sap_instance_number" do
    it "returns true for valid numbers" do
      expect(subject.sap_instance_number('05')).to eq true
      expect(subject.sap_instance_number('09')).to eq true
      expect(subject.sap_instance_number('10')).to eq true
      expect(subject.sap_instance_number('99')).to eq true
      end

    it "returns false for invalid numbers" do
      expect(subject.sap_instance_number('1')).to eq false
      expect(subject.sap_instance_number('1A')).to eq false
      expect(subject.sap_instance_number('999')).to eq false
      end
  end
end

Again, quick hack, please test it!

[varkoly]

Thanks a lot!
I've applied your suggestions in master.