file.uncomment does not uncomment as expected

[Akshaykapoor]

I want to uncomment a line from a config file,

# Use name-based virtual hosting.
#
# NameVirtualHost *:80 --> This line is to be uncommented
#
# NOTE: NameVirtualHost cannot be used without a port specifier
# (e.g. :80) if mod_ssl is being used, due to the nature of the
# SSL protocol.

In my salt state i'm writing the following,

/etc/httpd/conf/httpd.conf:
  file.uncomment:
    - regex: NameVirtualHost\W+\d+

This results in the a failure,

          ID: /etc/httpd/conf/httpd.conf
    Function: file.uncomment
      Result: False
     Comment: Expected uncommented lines not found
     Started: 18:40:32.721247
    Duration: 31.288 ms

Looking at the debug log, i came across this,

sed -i.bak -r -e '/NameVirtualHost\\W+\\d+/ s/^([[:space:]]*)#/\\1/g' /etc/httpd/conf/httpd.conf

which makes me think that i cannot use the different characters that i'm using '\W\d'. Not quite sure if this is a bug in salt but the my running it on CentOS 6.6 and salt version

           Salt: 2014.7.5
         Python: 2.6.6 (r266:84292, Jan 22 2014, 09:42:36)
         Jinja2: 2.2.1
       M2Crypto: 0.20.2
 msgpack-python: 0.4.6
   msgpack-pure: Not Installed
       pycrypto: 2.0.1
        libnacl: Not Installed
         PyYAML: 3.10
          ioflo: Not Installed
          PyZMQ: 14.3.1
           RAET: Not Installed
            ZMQ: 3.2.5
           Mako: Not Installed

How can i achieve the desired output using file.uncomment ?

[arroyoc]

@Akshaykapoor thanks for your report. Can you reproduce this on salt versions 2015.5.2?

[Akshaykapoor]

@arroyoc Will update the salt versions to that and get back with the results soon.

[MoonSweep]

I'm not sure if it's the same issue, but the error message makes me think it might be related.

I used to successfully uncomment a bunch of lines in /etc/vim/vimrc with a single file.uncomment snippet:

vim:
  file.uncomment:
    - name: /etc/vim/vimrc
    - char: '"'
    - regex: (syntax on|set (background=dark|showcmd|showmatch|ignorecase))

I'm pretty sure it used to work (at least on a first run, see #25345). Now with 2015.5.3, it doesn't work anymore:

----------
          ID: vim
    Function: file.uncomment
        Name: /etc/vim/vimrc
      Result: False
     Comment: Expected uncommented lines not found
     Started: 12:07:17.406047
    Duration: 36.188 ms
     Changes:   
              ----------
              diff:
                  --- 
                  +++ 
                  @@ -16,11 +16,11 @@

                   " Vim5 and later versions support syntax highlighting. Uncommenting the next
                   " line enables syntax highlighting by default.
                  -"syntax on
                  +(syntax on|set (background=dark|showcmd|showmatch|ignorecase))

                   " If using a dark background within the editing area and syntax highlighting
                   " turn on this option as well
                  -"set background=dark
                  +(syntax on|set (background=dark|showcmd|showmatch|ignorecase))

                   " Uncomment the following to have Vim jump to the last position when
                   " reopening a file
                  @@ -36,9 +36,9 @@

                   " The following are commented out as they cause vim to behave a lot
                   " differently from regular Vi. They are highly recommended though.
                  -"set showcmd     " Show (partial) command in status line.
                  -"set showmatch       " Show matching brackets.
                  -"set ignorecase      " Do case insensitive matching
                  +(syntax on|set (background=dark|showcmd|showmatch|ignorecase))       " Show (partial) command in status line.
                  +(syntax on|set (background=dark|showcmd|showmatch|ignorecase))       " Show matching brackets.
                  +(syntax on|set (background=dark|showcmd|showmatch|ignorecase))       " Do case insensitive matching
                   "set smartcase       " Do smart case matching
                   "set incsearch       " Incremental search
                   "set autowrite       " Automatically save before commands like :next and :make

It successfully detects which lines to uncomment, but instead of uncommenting, it replaces the matching pattern with the whole regexp.

If file.(un)comment are untrustworthy to the point that your usual answer is to advise using file.managed instead (like here or here), you could as well remove them from Salt completely and be done with it. I'd rather spend my time being creative with less tools (cmd.run sed commands always works) than waste my time fixing the mess and reporting issues about things that used to work and that I should be able to trust.

Seriously, this is an awful regression. It needs to be fixed ASAP.

[piotrooo]

👍

When this functionality will be back? This regression is really nasty.

[xcorvis]

I can confirm this in 2015.5.3. What other info is needed?

                  Salt: 2015.5.3
                Python: 2.7.3 (default, Jun 22 2015, 19:33:41)
                Jinja2: 2.6
              M2Crypto: 0.21.1
        msgpack-python: 0.1.10
          msgpack-pure: Not Installed
              pycrypto: 2.4.1
               libnacl: Not Installed
                PyYAML: 3.10
                 ioflo: Not Installed
                 PyZMQ: 14.0.1
                  RAET: Not Installed
                   ZMQ: 4.0.4
                  Mako: Not Installed
               Tornado: Not Installed
 Debian source package: 2015.5.3+ds-1precise2

[arroyoc]

@Akshaykapoor We shouldn't need anymore information. This issue has been labeled appropriately and will be taken care of as soon as we can get to it. Thanks for your help.

[russellballestrini]

Seems to still be an issue in 2016.11.1

[nhavens]

Here's an example of file.uncomment not working properly:

/tmp/default.pa

#load-sample-lazy x11-bell /usr/share/sounds/freedesktop/stereo/bell.oga
### Load X11 bell module
#load-module module-x11-bell sample=x11-bell

/srv/salt/uncomment.sls

uncomment x11 bell lines in pulse audio config:
  file.uncomment:
    - name: /tmp/default.pa
    - regex: ".*x11-bell.*"
    - backup: False

# salt '*minion*' state.apply uncomment
minion.local:
----------
          ID: uncomment x11 bell lines in pulse audio config
    Function: file.uncomment
        Name: /tmp/default.pa
      Result: True
     Comment: Pattern already uncommented
     Started: 15:49:10.981964
    Duration: 6.449 ms
     Changes:   

Summary for minion.local
------------
Succeeded: 1
Failed:    0
------------
Total states run:     1
Total run time:   6.449 ms

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[russellballestrini]

This is def still an issue, stale bot.

[stale]

Thank you for updating this issue. It is no longer marked as stale.

[nee2c]

I question if it is the state file.uncomment function and not the renderer that is rendering the state file that is the root of the problem. I had a similar problem and found it to be that the yaml was rendering the escape characters in the regex strings. I had a look at the code and sure I think the code could be simplified a bit but logic looks good. In yaml state file the regex needs to be in single quote or have the escape escaped eg. \\

The code that dose the heavy lifting of updating the file has been update about year ago so I think MoonSweep's issue is now mute.

For future reference nhavens example he has an wild card that will include the comment character and docs says don't include comment character. It is taking for example "#foox11-bellbar" to be uncommented due to wildcards and no matter what it will consider the line already uncommented.

So my take on this issue, that it has been all issues have been fixed or explained but docs could be updated to make things a little clearer.
I would put in a pull request updating docs with warnings to using wild cards and escape character.
Any thoughts?

[thatch45]

I am going to close this out and request a new issue be filed verifying this on the latest release. Assuming this is a real issue it should be fixed.
Please verify this bug and open a fresh issue so that we can start from a clean place given that the code almost certainly has changed since this issue was opened