file.comment issue when multiple lines with some already commented

[chris-be]

Description of Issue/Question

I try to comment some lines in a FreeBSD configuration file. This file contains some comment containing itself the pattern I want to find. Result is that nothing is done.
According to documentation "comment"/"uncomment" do not have a "flags" argument, would it be relevant to add it ?

Setup

Master(less) : Debian Stretch / virtualenv Python 2.7.13 (pip install salt-ssh) / salt-ssh 2016.11.5 (Carbon)
Minion (agentless) : FreeBSD 11.0 in virtualbox / Python 2.7.13

Steps to Reproduce Issue

First

On minion : create a file /home/test_user/test.conf :

# Test sample
*.whatever;and_also 	 /bad/news

# Don't forget to configure /bad/news before enabling it .. 

*.error /bad/news

On master : create a test.sls :

test-comment:
  file.comment:
    - name: /home/test_user/test.conf
    - regex: ^.*/bad/news\s*
    - backup: False

And add it to top.sls

base:
  'the_minion':
    - test

Than

salt-ssh 'the_minion' state.apply

What I get

salt-ssh 'the_minion' state.apply 
the_minion:
----------
          ID: test-comment
    Function: file.comment
        Name: /home/test_user/test.conf
      Result: True
     Comment: Pattern already commented
     Started: 20:11:39.976112
    Duration: 10.888 ms
     Changes:   

Summary for the_minion
------------
Succeeded: 1
Failed:    0
------------
Total states run:     1
Total run time:  10.888 ms

But if I replace "/bad/news" by "bAd/news" in line 4 of test.conf, I get :

salt-ssh 'the_minion' state.apply 
the_minion:
----------
          ID: test-comment
    Function: file.comment
        Name: /home/test_user/test.conf
      Result: True
     Comment: Commented lines successfully
     Started: 20:17:45.291284
    Duration: 14.342 ms
     Changes:   
              ----------
              diff:
                  --- 
                  +++ 
                  @@ -1,7 +1,7 @@
                   # Test sample
                  -*.whatever;and_also 	 /bad/news
                  +#*.whatever;and_also 	 /bad/news
                   
                   # Don't forget to configure /bAd/news before enabling it .. 
                   
                  -*.error /bad/news
                  +#*.error /bad/news
                   

Summary for the_minion
------------
Succeeded: 1 (changed=1)
Failed:    0
------------
Total states run:     1
Total run time:  14.342 ms

[garethgreenaway]

Looks like we need the ability to search and comment all instances of a particular expression. Marking this as approved. Thanks for the report!

[Arabus]

I think someone really went overboard with the error checking in this module.
"Let's run the regex over the file three times to see if we need to run the regex over the file to actually do something..." There ought to be a better way to do this.

[mbunkus]

I've just stumbled across this as well and agree that the checks done in file.py are completely overboard. The easiest way I can see is to iterate over all occurrences where the "to comment" regex matches, to comment them, and to set a flag according if at least one match is found. If the flag is still false, emit that "already commented" message.

Dito for file.uncomment.

Just get rid of that first "already commented?" regex search…

[mikesoule]

Any update on this one? Seems to still be an issue.

[diepes]

I got here searching for file.commet bugs.
In my case I don't have any match and get an error back, is that the expected result?
Comment: xyz: Pattern not found

It would make sense to me that un-comment throws an error if no match found, but the comment should not, or it should be made clear in the documentation.

Also, the regex seems to be anchored with an implicit "^" even if not supplied in the state regex.

[mbunkus]

@diepes I'm not a Salt developer myself, so take what I'm saying with a grain of salt (no pun intended). The behavior if the pattern isn't found at all matches the file.(un)comment module functions. They return False in that case, too. A discussion about whether the pattern not being present should be considered an error seems way out of scope of this particular issue — I'd recommend opening a new one instead.

[twangboy]

Thanks for the PR!