(MODULES-7758) Properly handle distro tag for Fedora platforms

There were some recent changes made to remove the 'f' prefix for our
older Fedora platforms, specifically Fedora 26 and 27. However, this
inadvertently changed our distro tag for these platforms to also remove
the 'f' prefix so that e.g. fedoraf26 became fedora26. This change was not
carried over to the puppet_agent module. Furthermore, newer Fedora
platforms like Fedora 28 use the newer fc<major_version> distro tag
for the puppet5 collection. The puppet_agent module was also not
updated with this change. Thus, some agent upgrades were failing on our
Fedora platforms.

This commit updates the puppet_agent module to handle all possible cases
of the distro tag for our Fedora platforms.

manifests/install.pp

@@ -165,7 +165,26 @@
     # Workaround PUP-5802/PUP-5025
     if ($::operatingsystem == 'Fedora') {
       if $pa_collection == 'PC1' or $pa_collection == 'puppet5' {
-        $dist_tag = "fedoraf${::operatingsystemmajrelease}"
+        # There's three cases here due to some mistakes with how we
+        # set-up our distro tags for Fedora platforms:
+        #   * For newer Fedora platforms (e.g. Fedora 28), we want
+        #     to use the fc<major> tag
+        #
+        #   * For older Fedora platforms (e.g. Fedora 26 and 27), we
+        #     have two separate cases:
+        #       * If the package version's > 5.5.3, then we use the fedora<major>
+        #         tag, b/c in those versions we removed the 'f' prefix.
+        #
+        #       * If the package version's <= 5.5.3, then we use the fedoraf<major>
+        #         tag b/c the 'f' prefix is still there.
+        #     
+        if (versioncmp("${::operatingsystemmajrelease}", '27') > 0) {
+          $dist_tag = "fc${::operatingsystemmajrelease}"
+        } elsif (versioncmp("${package_version}", '5.5.3') > 0) {
+          $dist_tag = "fedora${::operatingsystemmajrelease}"
+        } else {
+          $dist_tag = "fedoraf${::operatingsystemmajrelease}"
+        }
       } else {
         $dist_tag = "fc${::operatingsystemmajrelease}"
       }

spec/classes/puppet_agent_osfamily_redhat_spec.rb

@@ -2,23 +2,27 @@
 
 describe 'puppet_agent' do
   # All FOSS and all Puppet 4+ upgrades require the package_version
-  package_version = '1.2.5'
+  package_version = '5.5.4'
   let(:params) {
     {
       :package_version => package_version
     }
   }
 
-  [['Fedora', 'fedora/f$releasever', 7], ['CentOS', 'el/$releasever', 7], ['Amazon', 'el/6', 6]].each do |os, urlbit, osmajor|
+  let(:facts) do
+    {
+      :osfamily                  => 'RedHat',
+      :architecture              => 'x64',
+      :servername                => 'master.example.vm',
+      :clientcert                => 'foo.example.vm',
+    }
+  end
+
+  [['Fedora', 'fedora/f$releasever', 27], ['CentOS', 'el/$releasever', 7], ['Amazon', 'el/6', 6]].each do |os, urlbit, osmajor|
     context "with #{os} and #{urlbit}" do
-      let(:facts) {{
-        :osfamily                  => 'RedHat',
-        :operatingsystem           => os,
-        :architecture              => 'x64',
-        :servername                => 'master.example.vm',
-        :clientcert                => 'foo.example.vm',
-        :operatingsystemmajrelease => osmajor,
-      }}
+      let(:facts) do
+        super().merge(:operatingsystem  => os, :operatingsystemmajrelease => osmajor)
+      end
 
       it { is_expected.to contain_exec('import-GPG-KEY-puppetlabs').with({
         'path'      => '/bin:/usr/bin:/sbin:/usr/sbin',
@@ -116,6 +120,87 @@
     end
   end
 
+  # There are a lot of special cases here, so it is best to have a separate
+  # unit test context for them.
+  context 'distro tag on Fedora platforms' do
+    def sets_distro_tag_to(expected_distro_tag)
+      is_expected.to contain_package('puppet-agent').with_ensure(
+        "#{params[:package_version]}-1.#{expected_distro_tag}"
+      )
+    end
+
+    let(:facts) do
+      super().merge(:operatingsystem => 'Fedora')
+    end
+
+    context 'when the collection is PC1' do
+      let(:params) do
+        # Older agents use the PC1 collection, so set the package_version
+        # to 1.10.9 to simulate this.
+        super().merge(:collection => 'PC1', :package_version => '1.10.9')
+      end
+
+      let(:facts) do
+        # Older agents do not support anything past Fedora 27
+        super().merge(:operatingsystemmajrelease => 27)
+      end
+
+      it { sets_distro_tag_to('fedoraf27') }
+    end
+
+    context 'when the collection is puppet5' do
+      let(:params) do
+        super().merge(:collection => 'puppet5')
+      end
+
+      context 'on newer Fedora versions' do
+        let(:facts) do
+          super().merge(:operatingsystemmajrelease => 28)
+        end
+
+        it { sets_distro_tag_to('fc28') }
+      end
+
+      context 'on older Fedora versions' do
+        let(:facts) do
+          super().merge(:operatingsystemmajrelease => 27)
+        end
+
+        shared_examples 'a package version' do |package_version, distro_tag|
+          let(:params) do
+            super().merge(:package_version => package_version)
+          end
+
+          it { sets_distro_tag_to(distro_tag) }
+        end
+
+        context 'when package_version > 5.5.3' do
+          include_examples 'a package version', '5.5.4', 'fedora27'
+        end
+
+        context 'when package_version == 5.5.3' do
+          include_examples 'a package version', '5.5.3', 'fedoraf27'
+        end
+
+        context 'when package_version < 5.5.3' do
+          include_examples 'a package version', '5.5.1', 'fedoraf27'
+        end
+      end
+    end
+
+    context 'when the collection is newer than PC1 and puppet5' do
+      let(:params) do
+        super().merge(:collection => 'puppet6', :package_version => '6.0.0')
+      end
+
+      let(:facts) do
+        super().merge(:operatingsystemmajrelease => 27)
+      end
+
+      it { sets_distro_tag_to('fc27') }
+    end
+  end
+
   [['RedHat', 'el-7-x86_64', 'el-7-x86_64', 7], ['Amazon', '', 'el-6-x64', 6]].each do |os, tag, repodir, osmajor|
     context "when PE on #{os}" do
       before(:each) do
@@ -126,20 +211,18 @@
         end
 
         Puppet::Parser::Functions.newfunction(:pe_compiling_server_aio_build, :type => :rvalue) do |args|
-          '1.2.5'
+          '5.5.4'
         end
       end
 
-      let(:facts) {{
-        :osfamily                  => 'RedHat',
-        :operatingsystem           => os,
-        :architecture              => 'x64',
-        :servername                => 'master.example.vm',
-        :clientcert                => 'foo.example.vm',
-        :is_pe                     => true,
-        :platform_tag              => tag,
-        :operatingsystemmajrelease => osmajor,
-      }}
+      let(:facts) do
+        super().merge(
+          :operatingsystem  => os,
+          :operatingsystemmajrelease => osmajor,
+          :platform_tag => tag,
+          is_pe: true
+        )
+      end
 
       context 'with manage_repo enabled' do
         let(:params)  {
@@ -207,7 +290,7 @@
             :package_version => package_version
           }
         }
-        it { is_expected.to contain_package('puppet-agent').with_ensure("1.2.5-1.el#{osmajor}") }
+        it { is_expected.to contain_package('puppet-agent').with_ensure("#{params[:package_version]}-1.el#{osmajor}") }
 
       end