(maint) Centralize puppet_collection_for logic by ekinanp · Pull Request #100 · puppetlabs/beaker-puppet

ekinanp · 2019-01-24T00:07:56Z

This commit centralizes the puppet_collection_for_puppet_agent_version
and puppet_collection_for_puppet_version logic into a single
puppet_collection_for helper.

This commit centralizes the puppet_collection_for_puppet_agent_version and puppet_collection_for_puppet_version logic into a single puppet_collection_for helper.

ekinanp · 2019-01-24T00:11:39Z

-        def puppet_collection_for_puppet_agent_version(agent_version)
-          agent_version = agent_version.to_s
-          return 'puppet' if agent_version.strip == 'latest'
+        def puppet_collection_for(package, version)


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

melissa · 2019-01-24T00:24:34Z

+            return nil if x.nil? || y.nil? || z.nil?

-          return 'pc1' if x == 4
+            return 'pc1' if x == 4


do we need to keep pc1 around since we aren't doing anything with 1.x or 4.x in foss anymore?

I think so because we're still supporting upgrades from Puppet 4/Agent 1.x in the puppet_agent module. @caseywilliams can you confirm?

Is beaker-puppet used int he puppet_agent module? That's a little scary

oh wait, I shouldn't be surprised by this. The beaker puppet install helper uses beaker-puppet

However, wouldn't this only be needed if we were installing new package? This should be verified, but I'm not convinced we need this for packages that already exist on the system we are upgrading. Just a thought. It would be nice to start removing all the pc1 references in this code. It's totes something we can save for later.

We do need PC1 in the :puppet_agent package case because the puppet_agent module tests calculate the collection from the agent version. I don't know about PC1 in the :puppet package case though. @caseywilliams ?

EDIT: Nevermind. With the "reduce redundancy" commit, I think it's cleaner to keep the PC1 reference for Puppet 4.

👍 sounds good :) Safest to keep it

melissa · 2019-01-24T00:27:09Z

+        def puppet_collection_for(package, version)
+          valid_packages = [
+            :puppet_agent,
+            :puppet


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/

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.

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

melissa · 2019-01-24T00:29:14Z

-          "puppet#{x}" if x > 4
-        end
+            x, y, z = agent_version.to_s.split('.').map(&:to_i)
+            return nil if x.nil? || y.nil? || z.nil?


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).

Yeah, I can go ahead and do that.

Updated the PR with a new commit.

melissa · 2019-01-24T00:31:04Z

-          x, y, z = puppet_version.to_s.split('.').map(&:to_i)
-          return nil if x.nil? || y.nil? || z.nil?
+            x, y, z = puppet_version.to_s.split('.').map(&:to_i)
+            return nil if x.nil? || y.nil? || z.nil?


Hmm, I feel like we have enough information to return a valid result if the only thing we have is x, since it's only the major version that defines the release stream. Though, I suppose if y and z are nil that might indicate a malformed version string.... I'm not sure my hang up on this is worth any follow up work

We'll still need y to handle weird cases like the 5.99 tag (I forgot why we did that in the first place -- maybe @caseywilliams can clarify ?).

It might be worth raising an exception for a malformed version string?

I feel like raising an error isn't a bad idea if we can't figure out what's happening with the version string. Do we have existing utilities available to help parse version strings?

There is Beaker::Shared::Semvar, but that doesn't raise any errors if the versions are malformed strings and it isn't specific to semver parsing.

melissa

The travis failure isn't specific to this PR

18:09:14 An error occurred while installing i18n (1.5.3), and Bundler cannot continue.
18:09:14 Make sure that `gem install i18n -v '1.5.3' --source
18:09:14 'https://artifactory.delivery.puppetlabs.net/artifactory/api/gems/rubygems/'`
18:09:14 succeeds before bundling.
18:09:14 
18:09:14 In Gemfile:
18:09:14   markdown was resolved to 1.2.0, which depends on
18:09:14     textutils was resolved to 1.4.0, which depends on
18:09:14       activesupport was resolved to 5.2.2, which depends on
18:09:14         i18n

I'm going to dig into that now and hopefully fix it.

But this looks good to me! Thanks for answering all my questions!

There were some dangling references to the previous puppet_collection_for_<package> methods that should be removed.

ekinanp · 2019-01-24T18:50:19Z

Sorry, I realized there were some dangling references to the old puppet_collection_for_ methods. Pushed up a commit to remove them.

melissa · 2019-01-24T19:18:27Z

I manually edited the PR Tests job for this PR so I could test https://github.com/puppetlabs/ci-job-configs/pull/5530, but at least we know that's passing!

melissa · 2019-01-24T19:25:32Z

11:08:12 + bundle exec rake test:acceptance
11:08:13 rake aborted!
11:08:13 LoadError: cannot load such file -- rspec/core/rake_task

I'm digging into this locally to see what's up

caseywilliams

This is fine with me. Can I merge? Do we need to wait on acceptance?

melissa · 2019-01-24T21:28:15Z

Apparently acceptance has always failed

(maint) Centralize puppet_collection_for logic

2e3c1a6

This commit centralizes the puppet_collection_for_puppet_agent_version and puppet_collection_for_puppet_version logic into a single puppet_collection_for helper.

ekinanp requested a review from caseywilliams January 24, 2019 00:10

ekinanp commented Jan 24, 2019

View reviewed changes

melissa reviewed Jan 24, 2019

View reviewed changes

(maint) Clean-up redundant code in puppet_collection_for helper

d73f70b

melissa approved these changes Jan 24, 2019

View reviewed changes

(maint) Remove puppet_collection_for_<package> references

916441c

There were some dangling references to the previous puppet_collection_for_<package> methods that should be removed.

caseywilliams mentioned this pull request Jan 24, 2019

(MODULES-8348) Acceptance scaffold with beaker-puppet puppetlabs/puppetlabs-puppet_agent#371

Merged

caseywilliams approved these changes Jan 24, 2019

View reviewed changes

melissa merged commit 485d33c into puppetlabs:master Jan 24, 2019

Conversation

ekinanp commented Jan 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekinanp Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekinanp Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melissa left a comment

Choose a reason for hiding this comment

Uh oh!

ekinanp commented Jan 24, 2019

Uh oh!

melissa commented Jan 24, 2019

Uh oh!

melissa commented Jan 24, 2019

Uh oh!

caseywilliams left a comment

Choose a reason for hiding this comment

Uh oh!

melissa commented Jan 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ekinanp Jan 24, 2019 •

edited

Loading

ekinanp Jan 24, 2019 •

edited

Loading