Fix RemoveOldRepodata issues found in testing

Addresses the issues found in the testing issue 3816, including:

- minor documentation fixes to match expected behaviour
- remove_old_repodata is now applied as the default as the documents suggested
- setting the threshold for age of repodata files to 0, no longer publishes a repo with no content
- some cleaning up of code from an older implementation that is no longer needed

fixes #3816
https://pulp.plan.io/issues/3816

docs/tech-reference/yum-plugins.rst

@@ -752,15 +752,15 @@ Optional Configuration Parameters
  If unspecified the incremental publish will be performed when possible.
 
 ``remove_old_repodata``
- Boolean flag to indicate whether or not repodata past expiration are removed
- during publish. If not present it defaults to True. The default expiration is
- 14 days. Files that present in repomd.xml are preserved no matter of
- their age.
+ Boolean flag to indicate whether or not repodata that is past expiration will
+ be removed during publish. The age at which repodata is considered "old" is set
+ by the value of ``remove_old_repodata``. If not present this value defaults
+ to True.
 
 ``remove_old_repodata_threshold``
- If ``remove_old_repodata`` is true, this attribute specify maximal age of
- repodata in seconds that are not removed during publish. This attribute defaults
- to 14 days.
+ If ``remove_old_repodata`` is True, this attribute specifies the maximal age in
+ days of repodata files that are not removed during publish. This attribute
+ defaults to 14 days.
 
 GPG Signing of Repository Metadata
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

plugins/pulp_rpm/plugins/distributors/yum/metadata/repomd.py

@@ -90,7 +90,7 @@ def add_metadata_file_metadata(self, data_type, file_path, precalculated_checksu
         location = os.path.join(REPO_DATA_DIR_NAME, file_name)
         location_attributes = {'href': location}
         ElementTree.SubElement(data_element, 'location', location_attributes)
-        self.metadata_file_locations.append(location)
+        self.metadata_file_locations.append(file_name)
 
         timestamp_element = ElementTree.SubElement(data_element, 'timestamp')
         # Convert the float mtime to an integer before stringifying since

plugins/pulp_rpm/plugins/distributors/yum/publish.py

@@ -87,7 +87,7 @@ def __init__(self, repo, publish_conduit, config, distributor_type, association_
         self.add_child(PublishMetadataStep())
         self.add_child(CloseRepoMetadataStep())
         self.add_child(GenerateSqliteForRepoStep(self.get_working_dir()))
-        self.add_child(RemoveOldRepodataStep(self.get_working_dir()))
+        self.add_child(RemoveOldRepodataStep())
 
     def get_checksum_type(self):
         if not self.checksum_type:
@@ -1206,30 +1206,14 @@ class RemoveOldRepodataStep(platform_steps.PluginStep):
     """
     Remove repodata files that are not in repomd and are older than 14 days.
     """
-    def __init__(self, content_dir, **kwargs):
+    def __init__(self, **kwargs):
         """
-        Initialize the step for remove old repodata
-
-        :param content_dir: The base directory of the repository.  This directory should contain
-                            the repodata directory
-        :type content_dir: str
+        Initialize the step for removing old repodata
         """
         super(RemoveOldRepodataStep, self).__init__(
             constants.PUBLISH_REMOVE_OLD_REPODATA_STEP,
             **kwargs)
         self.description = _('Removing old repodata')
-        self.content_dir = content_dir
-
-    def is_skipped(self):
-        """
-        Check the repo for the config option to remove old repodata.
-        Clean up if remove_old_repodata is True, which is the default
-        behaviour.
-
-        :returns: return if step should clean old repodata or not
-        :rtype: bool
-        """
-        return not self.get_config().get('remove_old_repodata', True)
 
     def remove_repodata_file(self, repodata_file):
         os.remove(repodata_file)
@@ -1254,7 +1238,7 @@ def filter_old_repodata(self):
         for f in files_in_dir:
             delta = datetime.datetime.today() - \
                 datetime.datetime.fromtimestamp(os.path.getmtime(f))
-            if delta.total_seconds() > threshold and f not in to_keep:
+            if delta.total_seconds() > threshold and os.path.basename(f) not in to_keep:
                 to_remove.append(f)
 
         return to_remove
@@ -1263,8 +1247,10 @@ def process_main(self):
         """
         Check files times and remove ones older than 14 days.
         """
-        to_remove = self.filter_old_repodata()
-        for f in to_remove:
-            self.remove_repodata_file(f)
+        remove_old_repodata = self.get_config().get('remove_old_repodata', True)
+        if remove_old_repodata:
+            to_remove = self.filter_old_repodata()
+            for f in to_remove:
+                self.remove_repodata_file(f)
 
         self.total_units = self.progress_successes

plugins/test/unit/plugins/distributors/yum/metadata/test_metadata.py

@@ -520,10 +520,8 @@ def test_repomd_metadata_adds_metadata_file_locations(self, mock_getmtime,
                             REPO_DATA_DIR_NAME,
                             REPOMD_FILE_NAME)
 
-        file_list_name = os.path.basename(tempfile.NamedTemporaryFile().name)
-        update_list_name = os.path.basename(tempfile.NamedTemporaryFile().name)
-        file_list_path = os.path.join('repodata', file_list_name)
-        update_list_path = os.path.join('repodata', update_list_name)
+        file_list_path = os.path.basename(tempfile.NamedTemporaryFile().name)
+        update_list_path = os.path.basename(tempfile.NamedTemporaryFile().name)
 
         # file_locations in RepomdXMLFileContext adds
         expected_locations = [file_list_path, update_list_path]

plugins/test/unit/plugins/distributors/yum/test_publish.py

@@ -164,7 +164,7 @@ def test_publish(self, mock_publish_distribution, mock_publish_rpms, mock_publis
         mock_build_final_report.assert_called_once()
         mock_publish_comps.assert_called_once_with()
         mock_generate_sqlite.assert_called_once_with(self.working_dir)
-        mock_remove_old_repodata.assert_called_once_with(self.working_dir)
+        mock_remove_old_repodata.assert_called_once_with()
 
     @mock.patch('pulp_rpm.plugins.distributors.yum.publish.configuration.get_repo_checksum_type')
     def test_get_checksum_type(self, mock_get_checksum):
@@ -1405,18 +1405,13 @@ def test_data_path():
         repo_root = os.path.join(test_data_path(), self._testMethodName)
 
         self._init_publisher()
-        remove_step = publish.RemoveOldRepodataStep(repo_root)
+        remove_step = publish.RemoveOldRepodataStep()
         remove_step.parent = self.publisher
 
         metadata_file_path = os.path.join(repo_root, 'repodata', 'repomd.xml')
 
         # file_paths under plugins/test/data/test_remove_old_repodata "to_keep"
-        metadata_file_locations = [
-            metadata_file_path,
-            os.path.join(repo_root, 'repodata', 'repomd-tmp.xml'),
-            os.path.join(repo_root, 'repodata', 'repomd.xml.asc'),
-            os.path.join(repo_root, 'repodata', 'repomd-tmp.xml.asc')
-        ]
+        metadata_file_locations = ['repomd-tmp.xml', 'repomd.xml.asc', 'repomd-tmp.xml.asc']
 
         self.publisher.repomd_file_context.metadata_file_path = metadata_file_path
         self.publisher.repomd_file_context.metadata_file_locations = metadata_file_locations