-
Notifications
You must be signed in to change notification settings - Fork 13
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 Arch Linux support #15
Conversation
This is blocked on getting beaker, beaker-puppet and beaker-hostgenerator releases out. voxpupuli/beaker-hostgenerator#191 (comment) has a good overview of what's needed. After that, 👍 |
c4917c0
to
8a7a903
Compare
22ff65d
to
5f7b57e
Compare
@@ -54,7 +54,8 @@ def puppet_unit_test_matrix | |||
|
|||
def github_action_test_matrix(use_fqdn: false, pidfile_workaround: false) | |||
metadata.operatingsystems.each_with_object([]) do |(os, releases), matrix_include| | |||
releases&.each do |release| | |||
real_releases = ['Gentoo', 'Archlinux'].include?(os) ? ['rolling'] : releases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to avoid the duplication between here and the other 2 places. I'm not sure what the best place to capture it, but it feels like this should be captured in the metadata somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is now working . What do you think is the best to do deduplication? we could hack lib/puppet_metadata.rb
and set the release to rolling for Arch Linux and Gentoo. But that could confuse people if they assume the metadata object is 1:1 what's in the metadata.json
059da74
to
defe4ea
Compare
defe4ea
to
56f862f
Compare
@ekohl can you take a look here again as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you perhaps add some tests. I'd have expected it to fail here:
puppet_metadata/spec/metadata_spec.rb
Line 184 in 3977d94
describe 'beaker_setfiles' do |
Looks like that ensures certain values are yielded, but not that nothing else is yielded.
puppet_major_versions.each do |puppet_version| | ||
next unless AIO.has_aio_build?(os, release, puppet_version[:value]) | ||
# we currently support beaker jobs in GitHub only for operatingsystems with AIO packages from Puppet Inc or Archlinux | ||
next unless AIO.has_aio_build?(os, release, puppet_version[:value]) || os == 'Archlinux' && puppet_version[:value] == puppet_major_versions.map{|h| h[:value]}.sort.reverse.first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it doesn't make a huge difference, it's better to do puppet_major_versions.map{|h| h[:value]}.sort.reverse.first
outside of the loop. puppet_major_versions
doesn't change within it and that makes it a bit more efficient. It can also be more readable if you assign it a variable because right now it's quite a long line.
Also, I think braces around os == 'Archlinux' && ...
is clearer.
However, that does make me wonder if we should change the implementation to roughly thing:
metadata.operatingsystems.each_with_object([]) do |(os, releases), matrix_include|
case os
when 'Archlinux', 'Gentoo'
setfile = PuppetMetadata::Beaker.os_release_to_setfile(os, 'rolling', use_fqdn: use_fqdn, pidfile_workaround: pidfile_workaround)
if setfile
puppet_version = puppet_major_versions.max_by { |version| version[:value] }[:value]
matrix_include << construct_matrix_include(setfile, puppet_version)
end
else
releases.each do |release|
# Pretty much the current code but use construct_matrix_include
end
end
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will have a look at your suggestion later again. for now I created two methods in the metadata class to provide the lowest/highest puppet versions. They are quite short but could be useful for others as well. I just tested it in https://github.com/voxpupuli/puppet-borg/pull/90/checks and it's working as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused by construct_matrix_include()
. That method doesn't yet exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mhm
irb(main):011:0> m.construct_matrix_include(setfile, puppet_version)
=>
{:setfile=>
{:name=>"Archlinux rolling", :value=>"archlinuxrolling-64{hostname=archlinuxrolling-64.example.com}"},
:puppet=>7}
irb(main):012:0>
real_releases = ['Gentoo', 'Archlinux'].include?(os) ? ['rolling'] : releases | ||
real_releases&.each do |release| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, here you can make a case
statement and use yield
just once. It may be easier to follow.
043b990
to
4ca3143
Compare
4ca3143
to
8798d04
Compare
No description provided.