Merge pull request #38039 from rallytime/fix-37939

Check to see if a line is already commented before moving on

salt/states/file.py

@@ -3340,9 +3340,16 @@ def comment(name, regex, char='#', backup='.bak'):
         return _error(ret, check_msg)
 
     unanchor_regex = regex.lstrip('^').rstrip('$')
+    comment_regex = char + unanchor_regex
+
+    # Check if the line is already commented
+    if __salt__['file.search'](name, comment_regex, multiline=True):
+        commented = True
+    else:
+        commented = False
 
     # Make sure the pattern appears in the file before continuing
-    if not __salt__['file.search'](name, regex, multiline=True):
+    if commented or not __salt__['file.search'](name, regex, multiline=True):
         if __salt__['file.search'](name, unanchor_regex, multiline=True):
             ret['comment'] = 'Pattern already commented'
             ret['result'] = True

tests/integration/states/file.py

@@ -1233,6 +1233,11 @@ class FileTest(integration.ModuleCase, integration.SaltReturnAssertsMixIn):
             # write a line to file
             with salt.utils.fopen(name, 'w+') as fp_:
                 fp_.write('comment_me')
+
+            # Look for changes with test=True: return should be "None" at the first run
+            ret = self.run_state('file.comment', test=True, name=name, regex='^comment')
+            self.assertSaltNoneReturn(ret)
+
             # comment once
             ret = self.run_state('file.comment', name=name, regex='^comment')
             # result is positive
@@ -1248,6 +1253,11 @@ class FileTest(integration.ModuleCase, integration.SaltReturnAssertsMixIn):
             # line is still commented
             with salt.utils.fopen(name, 'r') as fp_:
                 self.assertTrue(fp_.read().startswith('#comment'))
+
+            # Test previously commented file returns "True" now and not "None" with test=True
+            ret = self.run_state('file.comment', test=True, name=name, regex='^comment')
+            self.assertSaltTrueReturn(ret)
+
         finally:
             os.remove(name)
 

tests/unit/states/file_test.py

@@ -989,7 +989,6 @@ class FileTestCase(TestCase):
 
         mock_t = MagicMock(return_value=True)
         mock_f = MagicMock(return_value=False)
-        mock = MagicMock(side_effect=[False, True, False, False])
         with patch.object(os.path, 'isabs', mock_f):
             comt = ('Specified file {0} is not an absolute path'.format(name))
             ret.update({'comment': comt, 'name': name})
@@ -997,8 +996,7 @@ class FileTestCase(TestCase):
 
         with patch.object(os.path, 'isabs', mock_t):
             with patch.dict(filestate.__salt__,
-                            {'file.contains_regex_multiline': mock,
-                             'file.search': mock}):
+                            {'file.search': MagicMock(side_effect=[True, True, True, False, False])}):
                 comt = ('Pattern already commented')
                 ret.update({'comment': comt, 'result': True})
                 self.assertDictEqual(filestate.comment(name, regex), ret)
@@ -1008,8 +1006,7 @@ class FileTestCase(TestCase):
                 self.assertDictEqual(filestate.comment(name, regex), ret)
 
             with patch.dict(filestate.__salt__,
-                            {'file.contains_regex_multiline': mock_t,
-                             'file.search': mock_t,
+                            {'file.search': MagicMock(side_effect=[False, True, False, True, True]),
                              'file.comment': mock_t,
                              'file.comment_line': mock_t}):
                 with patch.dict(filestate.__opts__, {'test': True}):