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

add --hosts param #111

wants to merge 1 commit into from

Conversation

jhoblitt
Copy link
Member

@jhoblitt jhoblitt commented Oct 26, 2023

@jhoblitt jhoblitt added the enhancement New feature or request label Oct 26, 2023
@jhoblitt jhoblitt marked this pull request as draft October 26, 2023 19:20
@jhoblitt
Copy link
Member Author

Is --roles the right name for this functionality as beaker has its own concept of a role?

@jhoblitt
Copy link
Member Author

It is starting to look like using a hostname prefix is a PITA for writing tests. It might be necessary to completely override the hostname.

@jhoblitt
Copy link
Member Author

The "host" matching doesn't seem to be purely literal as this is working locally for me:

# server/replica is only supported on Redhat
if fact_on('master', 'os.family') == 'RedHat'
  describe 'easy_ipa class' do
    include_examples 'the example', 'master.pp', 'master'
  end
end

if fact_on('client', 'os.family') == 'RedHat'
  describe 'easy_ipa class' do
    include_examples 'the example', 'client.pp', 'client'
  end
end

with:

BEAKER_destroy=no BEAKER_setfile=almalinux9-64{hostname=master-almalinux9-64.example.com}-almalinux9-64{hostname=client-almalinux9-64.example.com} BEAKER_PUPPET_COLLECTION=puppet7 bundle exec rake beaker

@jhoblitt jhoblitt changed the title (WIP) add --roles param (WIP) add --hosts param Oct 30, 2023
@jhoblitt
Copy link
Member Author

I've renamed --roles to --hosts and changed the semantics to provider a list of hostnames which completely override the "short hostname" of the SUTs.

@jhoblitt jhoblitt force-pushed the feature/multihost branch 2 times, most recently from 5191bb1 to 7b2a3ef Compare October 30, 2023 18:52
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Overall it looks good. Some small nit picks about style.

bin/metadata2gha Outdated Show resolved Hide resolved
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.

lib/puppet_metadata/beaker.rb Outdated Show resolved Hide resolved
lib/puppet_metadata/github_actions.rb Outdated Show resolved Hide resolved
@jhoblitt jhoblitt marked this pull request as ready for review October 30, 2023 21:26
@jhoblitt jhoblitt changed the title (WIP) add --hosts param add --hosts param Oct 30, 2023
@jhoblitt jhoblitt requested a review from ekohl November 27, 2023 23:17
@alexjfisher
Copy link
Member

Before I was directed to this PR, I spent some time today hacking away myself. To support some of the existing use cases some way of assigning roles would be helpful.
beaker-hostgenerator has a concept of arbitrary roles as well as a bunch of predefined ones.

But it starts to become a bit difficult to support everything in a single command line option with loads of different delimiters needed.
--hosts hostname1:compile_master,another_role.ma;hostname2:agent,another_role2.a

I think with the optparse library it also be possible to create an option that can be specified zero to multiple times.

eg maybe --host hostname1 --host hostname2:ROLE_STRING (where the first host isn't assigned any roles, but the second one uses a string that if present would be used verbatim eg. my_role.ma or just a)??

Did you have specific vox modules or other use-cases in mind? (The ones I came across were openvpn and spiped)

@jhoblitt
Copy link
Member Author

@alexjfisher My initial use case was acceptance testing for freeipa with coverage of a replica server and a client. lsst-it/puppet-ipa#3 This is a fork of https://forge.puppet.com/modules/puppetfinland/easy_ipa/readme after upstream removed the vox module plumbing. I would be more than happy to contribute this mod to vox...

I intentionally implemented the absolute minimum viable feature to get acceptance testing to run, in that server/replica/client can be tested for the same OS. There clearly needs to be more complicated configuration for mix role/os. As in freeipa server is only packaged for ELx these days. So it would be nice to be able to test EL9 servers with a Debian client. I think what is needed is the ability to configure a matrix of roles:os combos.

@h-haaks
Copy link
Contributor

h-haaks commented Apr 18, 2024

@jhoblitt Could you have a look at #124? It covers roles as well as hostnames.

@jhoblitt
Copy link
Member Author

@h-haaks I'm fine with it as long as @ekohl is. This PR was intended to be a first step to adding more functionality that would be easy to review. 😮‍💨

@ekohl
Copy link
Member

ekohl commented Apr 26, 2024

Since #124 is merged now, I'm going to close this.

@ekohl ekohl closed this Apr 26, 2024
@ekohl ekohl deleted the feature/multihost branch April 26, 2024 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants