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.

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

add --hosts param #111

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions bin/metadata2gha
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ options = {
domain: nil,
minimum_major_puppet_version: nil,
beaker_fact: nil,
hosts: nil,
}

OptionParser.new do |opts|
Expand Down Expand Up @@ -40,6 +41,9 @@ OptionParser.new do |opts|
options[:beaker_facter] = [fact, label, values.split(',')]
end
end
opts.on('--hosts VALUE,VALUE', Array, 'A list of hostnames to create -- generates multiple SUTs. Separate values using commas') do |opt|
options[:hosts] = opt unless opt.empty?
end
end.parse!

filename = ARGV[0]
Expand Down
21 changes: 17 additions & 4 deletions lib/puppet_metadata/beaker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,20 +55,33 @@ def adjusted_os(os)
# Enforce a domain to be appended to the hostname, making it an FQDN
# @param [Optional[String]] puppet_version
# The desired puppet version. Will be appended to the hostname
# @param [Optional[String]] hostname
# Override the automatically generated hostname. The domain may still
# be set via the `domain` param.
#
# @return [nil] If no setfile is available
# @return [Array<(String, String)>] The beaker setfile description with a readable name
def os_release_to_setfile(os, release, use_fqdn: false, pidfile_workaround: false, domain: nil, puppet_version: nil)
def os_release_to_setfile(os, release, use_fqdn: false, pidfile_workaround: false, domain: nil, puppet_version: nil, hostname: nil)
return unless os_supported?(os)

aos = adjusted_os(os)

name = "#{aos}#{release.tr('.', '')}-64"
hostname = (puppet_version.nil? && puppet_version != 'none') ? name : "#{name}-#{puppet_version}"
shostname = if hostname
hostname
elsif !puppet_version.nil?
"#{name}-#{puppet_version}"
else
name
end

domain ||= 'example.com' if use_fqdn

options = {}
options[:hostname] = "#{hostname}.#{domain}" if domain
if domain
options[:hostname] = "#{shostname}.#{domain}"
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps bring the shostname declaration to within this block?

Copy link
Member Author

Choose a reason for hiding this comment

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

that shouldn't break any of the existing tests but I suspect the logic here is wrong... the hostname should be set even when there isn't a domain component.

Copy link
Member Author

Choose a reason for hiding this comment

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

This probably makes more sense but would require updating 5 existing tests:

diff --git a/lib/puppet_metadata/beaker.rb b/lib/puppet_metadata/beaker.rb
index bdf3955..e5b38cf 100644
--- a/lib/puppet_metadata/beaker.rb
+++ b/lib/puppet_metadata/beaker.rb
@@ -63,7 +63,7 @@ module PuppetMetadata
 
         aos = adjusted_os(os)
         name = "#{aos}#{release.tr('.', '')}-64"
-        shostname = if hostname
+        full_hostname = if hostname
                       hostname
                     elsif !puppet_version.nil?
                       "#{name}-#{puppet_version}"
@@ -72,12 +72,11 @@ module PuppetMetadata
                     end
 
         domain ||= 'example.com' if use_fqdn
+        full_hostname += ".#{domain}" if domain
 
         options = {}
-        if domain
-          options[:hostname] = "#{shostname}.#{domain}"
-        elsif hostname
-          options[:hostname] = hostname
+        if full_hostname != name
+          options[:hostname] = full_hostname
         end
 
         # Docker messes up cgroups and some systemd versions can't deal with

However, I'm leaning towards leaving it alone as I feel like a bigger picture rethink might be called for.

elsif hostname
options[:hostname] = hostname
end

# Docker messes up cgroups and some systemd versions can't deal with
# that when PIDFile is used.
Expand Down
35 changes: 27 additions & 8 deletions lib/puppet_metadata/github_actions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -137,14 +137,33 @@ def puppet_version_below_minimum?(version)
end

def os_release_to_beaker_setfile(os, release, puppet_collection)
PuppetMetadata::Beaker.os_release_to_setfile(
os,
release,
use_fqdn: options[:beaker_use_fqdn],
pidfile_workaround: options[:beaker_pidfile_workaround],
domain: options[:domain],
puppet_version: puppet_collection,
)
if options[:hosts]
setfiles = options[:hosts].filter_map do |host|
PuppetMetadata::Beaker.os_release_to_setfile(
os,
release,
use_fqdn: options[:beaker_use_fqdn],
pidfile_workaround: options[:beaker_pidfile_workaround],
domain: options[:domain],
puppet_version: puppet_collection,
hostname: host,
)
end

return if setfiles.empty?

# merge the setfile strings
[setfiles.collect { |setfile| setfile[0] }.join('-'), setfiles[0][1]]
else
PuppetMetadata::Beaker.os_release_to_setfile(
os,
release,
use_fqdn: options[:beaker_use_fqdn],
pidfile_workaround: options[:beaker_pidfile_workaround],
domain: options[:domain],
puppet_version: puppet_collection,
)
end
end
end
end
13 changes: 13 additions & 0 deletions spec/beaker_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -57,5 +57,18 @@
it { expect(described_class.os_release_to_setfile('CentOS', '8', pidfile_workaround: ['CentOS'])).to be_nil }
end
end

describe 'hostname' do
describe 'without domain' do
it { expect(described_class.os_release_to_setfile('AlmaLinux', '9', hostname: 'foo')).to eq(['almalinux9-64{hostname=foo}', 'AlmaLinux 9']) }
end

describe 'with domain' do
it {
expect(described_class.os_release_to_setfile('AlmaLinux', '9', domain: 'example.com',
hostname: 'foo')).to eq(['almalinux9-64{hostname=foo.example.com}', 'AlmaLinux 9'])
}
end
end
end
end
20 changes: 20 additions & 0 deletions spec/github_actions_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,16 @@
)
end
end

context 'with hosts option' do
let(:options) { super().merge(hosts: %w[foo bar baz]) }

it 'is expected to contain supported os / puppet version / role combinations' do
expect(subject).to include(
{ name: 'Puppet 8 - CentOS 9', env: { 'BEAKER_PUPPET_COLLECTION' => 'puppet8', 'BEAKER_SETFILE' => 'centos9-64{hostname=foo}-centos9-64{hostname=bar}-centos9-64{hostname=baz}' } },
)
end
end
end

describe 'github_action_test_matrix' do
Expand Down Expand Up @@ -289,6 +299,16 @@
)
end
end

context 'with hosts option' do
let(:options) { super().merge(hosts: %w[foo bar baz]) }

it 'is expected to contain supported os / puppet version combinations with hostname option' do
expect(subject).to include(
{ name: 'Puppet 8 - CentOS 9', setfile: { name: 'CentOS 9', value: 'centos9-64{hostname=foo}-centos9-64{hostname=bar}-centos9-64{hostname=baz}' }, puppet: { name: 'Puppet 8', value: 8, collection: 'puppet8' } },
)
end
end
end
end
# rubocop:enable Layout/LineLength,RSpec/ExampleLength
Expand Down