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.

Sign up for GitHub

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

(maint) Centralize puppet_collection_for logic #100

Merged
merged 3 commits into from
Jan 24, 2019
Merged

(maint) Centralize puppet_collection_for logic #100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2e3c1a6
(maint) Centralize puppet_collection_for logic
ekinanp Jan 23, 2019
d73f70b
(maint) Clean-up redundant code in puppet_collection_for helper
ekinanp Jan 24, 2019
916441c
(maint) Remove puppet_collection_for_<package> references
ekinanp Jan 24, 2019
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
2 changes: 1 addition & 1 deletion lib/beaker-puppet/install_utils/foss_utils.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ def install_puppet_on(hosts, opts = options)
def install_puppet_agent_on(hosts, opts = {})
opts = FOSS_DEFAULT_DOWNLOAD_URLS.merge(opts)
opts[:puppet_agent_version] ||= opts[:version] #backwards compatability with old parameter name
opts[:puppet_collection] ||= puppet_collection_for_puppet_agent_version(opts[:puppet_agent_version]) || 'pc1'
opts[:puppet_collection] ||= puppet_collection_for(:puppet_agent, opts[:puppet_agent_version]) || 'pc1'

opts[:puppet_collection].downcase! # the collection names are case sensitive

Expand Down
58 changes: 28 additions & 30 deletions lib/beaker-puppet/install_utils/puppet_utils.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,7 @@ def remove_puppet_paths_on(hosts)
#
# @param [String] agent_version version string or 'latest'
# @deprecated This method returns 'PC1' as the latest puppet collection;
# this is incorrect. Use {#puppet_collection_for_puppet_agent_version} or
# {#puppet_collection_for_puppet_version} instead.
# this is incorrect. Use {#puppet_collection_for} instead.
def get_puppet_collection(agent_version = 'latest')
collection = "PC1"
if agent_version != 'latest'
Expand All @@ -90,41 +89,40 @@ def get_puppet_collection(agent_version = 'latest')
collection
end

# Determine the puppet collection that matches a given version of the puppet-agent
# package (you can find this version in the `aio_agent_version` fact).
# Determine the puppet collection that matches a given package version. The package
# must be one of
# * :puppet_agent (you can get this version from the `aio_agent_version_fact`)
# * :puppet
#
# @param agent_version [String] a semver version number of the puppet-agent package, or the string 'latest'
#
# @param package [Symbol] the package name. must be one of :puppet_agent, :puppet
# @param version [String] a version number or the string 'latest'
# @returns [String|nil] the name of the corresponding puppet collection, if any
def puppet_collection_for_puppet_agent_version(agent_version)
agent_version = agent_version.to_s
return 'puppet' if agent_version.strip == 'latest'

x, y, z = agent_version.to_s.split('.').map(&:to_i)
return nil if x.nil? || y.nil? || z.nil?
def puppet_collection_for(package, version)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chose this instead of puppet_collection_for(agent_version: <version>) because a single entry hash parameter doesn't really make sense.

valid_packages = [
:puppet_agent,
:puppet
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we have a longer list of valid packages here? Puppetserver, puppetdb, puppetdb-termini are all versioned on the same scheme as puppet and puppet_agent, so there is a case to be made that we might need to find the release stream for those packages.

http://apt.puppet.com/pool/jessie/puppet6/p/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking this could be something that'd be extended on a per-need case? So if a puppetserver person finds out that they need to calculate the collection from their package version, then they can extend this method with that code (and they'd figure out they need the collection from the raised exception). If you think it is a good idea to do as much as we can in the current implementation, however, I'm happy to update it. I'm not sure what the collection should be for legacy server/puppetdb versions (like Server 2.x, since agent 1.x uses it). Maybe we don't need to worry about those.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose it depends on where this method is used? But generally since this repo is so ad hoc, I'm fine waiting to add those other packages until we need them. As long as that's a conscious decision

]

return 'pc1' if x == 1

# A y version >= 99 indicates a pre-release version of the next x release
x += 1 if y >= 99
"puppet#{x}" if x > 4
end
unless valid_packages.include?(package)
raise "package must be one of #{valid_packages.join(', ')}"
end

# Determine the puppet collection that matches a given version of the puppet gem.
#
# @param version [String] a semver version number of the puppet gem, or the string 'latest'
# @returns [String|nil] the name of the corresponding puppet collection, if any
def puppet_collection_for_puppet_version(puppet_version)
puppet_version = puppet_version.to_s
return 'puppet' if puppet_version.strip == 'latest'
case package
when :puppet_agent, :puppet
version = version.to_s
return 'puppet' if version.strip == 'latest'

x, y, z = puppet_version.to_s.split('.').map(&:to_i)
return nil if x.nil? || y.nil? || z.nil?
x, y, z = version.to_s.split('.').map(&:to_i)
return nil if x.nil? || y.nil? || z.nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way we can reduce the redundancies here? The only real difference between :puppet_agent and :puppet that I can tell is the pc1 logic (return 'pc1' if x == 1 vs return 'pc1' if x == 4).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I can go ahead and do that.

Copy link
Contributor Author

@ekinanp ekinanp Jan 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Updated the PR with a new commit.


return 'pc1' if x == 4
pc1_x = package == :puppet ? 4 : 1
return 'pc1' if x == pc1_x

# A y version >= 99 indicates a pre-release version of the next x release
x += 1 if y >= 99
"puppet#{x}" if x > 4
# A y version >= 99 indicates a pre-release version of the next x release
x += 1 if y >= 99
"puppet#{x}" if x > 4
end
end

# Report the version of puppet-agent installed on `host`
Expand Down
101 changes: 53 additions & 48 deletions spec/beaker-puppet/install_utils/puppet_utils_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,64 +137,69 @@ def logger

end

describe '#puppet_collection_for_puppet_agent_version' do
context 'given a valid version of puppet-agent' do
{
'1.10.14' => 'pc1',
'1.10.x' => 'pc1',
'5.3.1' => 'puppet5',
'5.3.x' => 'puppet5',
'5.99.0' => 'puppet6',
'6.1.99-foo' => 'puppet6',
'6.99.99' => 'puppet7',
'7.0.0' => 'puppet7',
}.each do |version, collection|
it "returns collection '#{collection}' for version '#{version}'" do
expect(subject.puppet_collection_for_puppet_agent_version(version)).to eq(collection)
describe '#puppet_collection_for' do
it 'raises an error when given an invalid package' do
expect { subject.puppet_collection_for(:foo, '5.5.4') }.to raise_error
end

context 'when the :puppet_agent package is passed in' do
context 'given a valid version' do
{
'1.10.14' => 'pc1',
'1.10.x' => 'pc1',
'5.3.1' => 'puppet5',
'5.3.x' => 'puppet5',
'5.99.0' => 'puppet6',
'6.1.99-foo' => 'puppet6',
'6.99.99' => 'puppet7',
'7.0.0' => 'puppet7',
}.each do |version, collection|
it "returns collection '#{collection}' for version '#{version}'" do
expect(subject.puppet_collection_for(:puppet_agent, version)).to eq(collection)
end
end
end
end

it "returns the default, latest puppet collection given the version 'latest'" do
expect(subject.puppet_collection_for_puppet_agent_version('latest')).to eq('puppet')
end


context 'given an invalid version of puppet-agent' do
[nil, '', '0.1.0', '3.8.1', '', 'not-semver', 'not.semver.either'].each do |version|
it "returns a nil collection value for version '#{version}'" do
expect(subject.puppet_collection_for_puppet_agent_version(version)).to be_nil

it "returns the default, latest puppet collection given the version 'latest'" do
expect(subject.puppet_collection_for(:puppet_agent, 'latest')).to eq('puppet')
end

context 'given an invalid version' do
[nil, '', '0.1.0', '3.8.1', '', 'not-semver', 'not.semver.either'].each do |version|
it "returns a nil collection value for version '#{version}'" do
expect(subject.puppet_collection_for(:puppet_agent, version)).to be_nil
end
end
end
end
end

describe '#puppet_collection_for_puppet_version' do
context 'given a valid version of puppet' do
{
'4.9.0' => 'pc1',
'4.10.x' => 'pc1',
'5.3.1' => 'puppet5',
'5.3.x' => 'puppet5',
'5.99.0' => 'puppet6',
'6.1.99-foo' => 'puppet6',
'6.99.99' => 'puppet7',
'7.0.0' => 'puppet7',
}.each do |version, collection|
it "returns collection '#{collection}' for version '#{version}'" do
expect(subject.puppet_collection_for_puppet_version(version)).to eq(collection)
context 'when the :puppet package is passed-in' do
context 'given a valid version' do
{
'4.9.0' => 'pc1',
'4.10.x' => 'pc1',
'5.3.1' => 'puppet5',
'5.3.x' => 'puppet5',
'5.99.0' => 'puppet6',
'6.1.99-foo' => 'puppet6',
'6.99.99' => 'puppet7',
'7.0.0' => 'puppet7',
}.each do |version, collection|
it "returns collection '#{collection}' for version '#{version}'" do
expect(subject.puppet_collection_for(:puppet, version)).to eq(collection)
end
end
end
end

it "returns the default, latest puppet collection given the version 'latest'" do
expect(subject.puppet_collection_for_puppet_version('latest')).to eq('puppet')
end
it "returns the default, latest puppet collection given the version 'latest'" do
expect(subject.puppet_collection_for(:puppet, 'latest')).to eq('puppet')
end

context 'given an invalid version of puppet' do
[nil, '', '0.1.0', '3.8.1', '', 'not-semver', 'not.semver.either'].each do |version|
it "returns a nil collection value for version '#{version}'" do
expect(subject.puppet_collection_for_puppet_version(version)).to be_nil
context 'given an invalid version' do
[nil, '', '0.1.0', '3.8.1', '', 'not-semver', 'not.semver.either'].each do |version|
it "returns a nil collection value for version '#{version}'" do
expect(subject.puppet_collection_for(:puppet, version)).to be_nil
end
end
end
end
Expand Down