Merge pull request #46531 from terminalmage/issue44299

Fix regression in yumpkg._parse_repo_file()

salt/modules/yumpkg.py

@@ -2663,20 +2663,18 @@ def mod_repo(repo, basedir=None, **kwargs):
     filerepos[repo].update(repo_opts)
     content = header
     for stanza in six.iterkeys(filerepos):
-        comments = ''
-        if 'comments' in six.iterkeys(filerepos[stanza]):
-            comments = salt.utils.pkg.rpm.combine_comments(
-                    filerepos[stanza]['comments'])
-            del filerepos[stanza]['comments']
-        content += '\n[{0}]'.format(stanza)
+        comments = salt.utils.pkg.rpm.combine_comments(
+            filerepos[stanza].pop('comments', [])
+        )
+        content += '[{0}]\n'.format(stanza)
         for line in six.iterkeys(filerepos[stanza]):
-            content += '\n{0}={1}'.format(
+            content += '{0}={1}\n'.format(
                 line,
                 filerepos[stanza][line]
                     if not isinstance(filerepos[stanza][line], bool)
                     else _bool_to_str(filerepos[stanza][line])
             )
-        content += '\n{0}\n'.format(comments)
+        content += comments + '\n'
 
     with salt.utils.fopen(repofile, 'w') as fileout:
         fileout.write(content)
@@ -2704,14 +2702,29 @@ def _parse_repo_file(filename):
         section_dict.pop('__name__', None)
         config[section] = section_dict
 
-    # Try to extract leading comments
+    # Try to extract header comments, as well as comments for each repo. Read
+    # from the beginning of the file and assume any leading comments are
+    # header comments. Continue to read each section header and then find the
+    # comments for each repo.
     headers = ''
-    with salt.utils.fopen(filename, 'r') as rawfile:
-        for line in rawfile:
-            if line.strip().startswith('#'):
-                headers += '{0}\n'.format(line.strip())
-            else:
-                break
+    section = None
+    with salt.utils.fopen(filename, 'r') as repofile:
+        for line in repofile:
+            line = line.strip()
+            if line.startswith('#'):
+                if section is None:
+                    headers += line + '\n'
+                else:
+                    try:
+                        comments = config[section].setdefault('comments', [])
+                        comments.append(line[1:].lstrip())
+                    except KeyError:
+                        log.debug(
+                            'Found comment in %s which does not appear to '
+                            'belong to any repo section: %s', filename, line
+                        )
+            elif line.startswith('[') and line.endswith(']'):
+                section = line[1:-1]
 
     return (headers, config)
 

salt/utils/pkg/rpm.py

@@ -11,7 +11,6 @@ import subprocess
 
 # Import 3rd-party libs
 from salt.ext import six
-from salt.ext.six.moves import range  # pylint: disable=redefined-builtin
 
 log = logging.getLogger(__name__)
 
@@ -112,10 +111,10 @@ def combine_comments(comments):
     '''
     if not isinstance(comments, list):
         comments = [comments]
-    for idx in range(len(comments)):
-        if not isinstance(comments[idx], six.string_types):
-            comments[idx] = str(comments[idx])
-        comments[idx] = comments[idx].strip()
-        if not comments[idx].startswith('#'):
-            comments[idx] = '#' + comments[idx]
-    return '\n'.join(comments)
+    ret = []
+    for comment in comments:
+        if not isinstance(comment, six.string_types):
+            comment = str(comment)
+        # Normalize for any spaces (or lack thereof) after the #
+        ret.append('# {0}\n'.format(comment.lstrip('#').lstrip()))
+    return ''.join(ret)

tests/integration/states/test_pkgrepo.py

@@ -22,16 +22,16 @@ import salt.utils
 import salt.ext.six as six
 
 
+@destructiveTest
+@skipIf(salt.utils.is_windows(), 'minion is windows')
 class PkgrepoTest(ModuleCase, SaltReturnAssertsMixin):
     '''
     pkgrepo state tests
     '''
-    @destructiveTest
-    @skipIf(salt.utils.is_windows(), 'minion is windows')
     @requires_system_grains
     def test_pkgrepo_01_managed(self, grains):
         '''
-        This is a destructive test as it adds a repository.
+        Test adding a repo
         '''
         os_grain = self.run_function('grains.item', ['os'])['os']
         os_release_info = tuple(self.run_function('grains.item', ['osrelease_info'])['osrelease_info'])
@@ -56,12 +56,9 @@ class PkgrepoTest(ModuleCase, SaltReturnAssertsMixin):
         for state_id, state_result in six.iteritems(ret):
             self.assertSaltTrueReturn(dict([(state_id, state_result)]))
 
-    @destructiveTest
-    @skipIf(salt.utils.is_windows(), 'minion is windows')
     def test_pkgrepo_02_absent(self):
         '''
-        This is a destructive test as it removes the repository added in the
-        above test.
+        Test removing the repo from the above test
         '''
         os_grain = self.run_function('grains.item', ['os'])['os']
         os_release_info = tuple(self.run_function('grains.item', ['osrelease_info'])['osrelease_info'])
@@ -78,3 +75,56 @@ class PkgrepoTest(ModuleCase, SaltReturnAssertsMixin):
         self.assertReturnNonEmptySaltType(ret)
         for state_id, state_result in six.iteritems(ret):
             self.assertSaltTrueReturn(dict([(state_id, state_result)]))
+
+    @requires_system_grains
+    def test_pkgrepo_03_with_comments(self, grains):
+        '''
+        Test adding a repo with comments
+        '''
+        os_family = grains['os_family'].lower()
+
+        if os_family in ('redhat', 'suse'):
+            kwargs = {
+                'name': 'examplerepo',
+                'baseurl': 'http://example.com/repo',
+                'enabled': False,
+                'comments': ['This is a comment']
+            }
+        elif os_family in ('debian',):
+            self.skipTest('Debian/Ubuntu test case needed')
+        else:
+            self.skipTest("No test case for os_family '{0}'".format(os_family))
+
+        try:
+            # Run the state to add the repo
+            ret = self.run_state('pkgrepo.managed', **kwargs)
+            self.assertSaltTrueReturn(ret)
+
+            # Run again with modified comments
+            kwargs['comments'].append('This is another comment')
+            ret = self.run_state('pkgrepo.managed', **kwargs)
+            self.assertSaltTrueReturn(ret)
+            ret = ret[next(iter(ret))]
+            self.assertEqual(
+                ret['changes'],
+                {
+                    'comments': {
+                        'old': ['This is a comment'],
+                        'new': ['This is a comment',
+                                'This is another comment']
+                    }
+                }
+            )
+
+            # Run a third time, no changes should be made
+            ret = self.run_state('pkgrepo.managed', **kwargs)
+            self.assertSaltTrueReturn(ret)
+            ret = ret[next(iter(ret))]
+            self.assertFalse(ret['changes'])
+            self.assertEqual(
+                ret['comment'],
+                "Package repo '{0}' already configured".format(kwargs['name'])
+            )
+        finally:
+            # Clean up
+            self.run_state('pkgrepo.absent', name=kwargs['name'])