Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: file.blockreplace inserts additional blank line on multi-line content #12422

Closed
creaky opened this issue Apr 30, 2014 · 24 comments

Comments

Projects
None yet
8 participants
@creaky
Copy link
Contributor

commented Apr 30, 2014

Bug
file.blockreplace will introduce an unwanted final new line when given multi-line content.

Patch to correct this bug has been included at end.

Cause
The behaviour of split when the separating string is provided is to return a list including one more item than the number of occurrences of the separator in the string.

In the original code this means a line with a terminating new line character at the end will return a list of two items being the line content and the terminating line character. As the original code then introduces a new line for each item in the list, the unwanted final new line is thus produced.

Reproduce

test:
  file.blockreplace:
    - name: /tmp/testfile
    - marker_start: "# STARTMARKER"
    - marker_end: "# ENDMARKER"
    - content: |
        # TEST LINE 1
        # TEST LINE 2
    - backup: False

Expected Output

# STARTMARKER
# TEST LINE 1
# TEST LINE 2
# ENDMARKER

Actual Output

# STARTMARKER
# TEST LINE 1
# TEST LINE 2

# ENDMARKER

Patch

--- ./salt/modules/file.py.orig 2014-04-17 03:25:04.000000000 +0000
+++ ./salt/modules/file.py  2014-04-30 17:27:12.000000000 +0000
@@ -1110,6 +1110,11 @@
                     # end of block detected
                     in_block = False

+                    # Check for multi-line '\n' terminated content as split will
+                    # introduce an unwanted additional new line.
+                    if content[-1] == '\n':
+                        content = content[:-1]
+
                     # push new block content in file
                     for cline in content.split("\n"):
                         new_file.append(cline + "\n")
@basepi

This comment has been minimized.

Copy link
Collaborator

commented Apr 30, 2014

Would you mind submitting a pull request, @creaky? Since you already have a handle on the patch?

@basepi basepi added Bug labels Apr 30, 2014

@basepi basepi added this to the Approved milestone Apr 30, 2014

creaky added a commit to creaky/salt that referenced this issue May 4, 2014

Fix Issue: saltstack#12422 blockreplace adding blank lines
file.blockreplace introduced an unwanted final new line when given
multi-line content due to split() behaviour.

thatch45 added a commit that referenced this issue May 5, 2014

Merge pull request #12522 from creaky/issue12422
Fix Issue: #12422 blockreplace adding blank lines

basepi added a commit that referenced this issue Jun 6, 2014

Fix Issue: #12422 blockreplace adding blank lines
file.blockreplace introduced an unwanted final new line when given
multi-line content due to split() behaviour.
@jfindlay

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2015

It looks like this has been fixed, so I am closing this issue.

@jfindlay jfindlay closed this Jul 28, 2015

@suslikas

This comment has been minimized.

Copy link

commented Aug 3, 2015

Not fixed in salt 2015.5.2 (Lithium). Same problem, on first run you get blank line, on second line removed and then work fine.

@basepi basepi reopened this Aug 3, 2015

@djneades

This comment has been minimized.

Copy link

commented Sep 7, 2015

I’ve just run into this bug with salt 2015.5.5, too.

@steverweber

This comment has been minimized.

Copy link
Contributor

commented May 4, 2016

+1 this bug is frustrating!
This issue can cause watched services to restart a second time because of newline diff.

@thatch45

This comment has been minimized.

Copy link
Member

commented May 4, 2016

@steverweber what version are you on? I can't reproduce this on 2016.3 or on 2015.8. Can you give a case that causes this to happen?

@steverweber

This comment has been minimized.

Copy link
Contributor

commented May 4, 2016

odyssey@odyssey-feed-1:/$ salt-call --version
salt-call 2015.5.9 (Lithium)
{% load_yaml as vars %}
contents: |
   bla
   bla
   bla
{% endload %}

{{ sls }} file:
    file.managed:
        - name: /test
        - mode: 0400

{{ sls }} block:
    file.blockreplace:
        - name: /test
        - marker_start: "### START zone SALT ###"
        - marker_end: "### END zone SALT ###"
        - content: {{ ('#{0}\n'.format(sls) + vars.contents)|json }}
        - append_if_not_found: True
        - show_changes: True

state.highstate

----------
          ID: uw.service.odyssey.01.feed file
    Function: file.managed
        Name: /test
      Result: True
     Comment: Empty file
     Started: 12:49:41.073296
    Duration: 4.04 ms
     Changes:   
              ----------
              mode:
                  0400
              new:
                  file /test created
----------
          ID: uw.service.odyssey.01.feed block
    Function: file.blockreplace
        Name: /test
      Result: True
     Comment: Changes were made
     Started: 12:49:41.077436
    Duration: 1.346 ms
     Changes:   
              ----------
              diff:
                  --- 
                  +++ 
                  @@ -0,0 +1,3 @@
                  +### START zone SALT ###
                  +#uw.service.odyssey.01.feed
                  bla
                  bla
                  bla

                  +### END zone SALT ###

also note the diff looks strange bla should have a + bla

state.highstate (run2)

          ID: uw.service.odyssey.01.feed file
    Function: file.managed
        Name: /test
      Result: True
     Comment: File /test exists with proper permissions. No changes made.
     Started: 12:50:36.160531
    Duration: 0.711 ms
     Changes:   
----------
          ID: uw.service.odyssey.01.feed block
    Function: file.blockreplace
        Name: /test
      Result: True
     Comment: Changes were made
     Started: 12:50:36.161325
    Duration: 1.735 ms
     Changes:   
              ----------
              diff:
                  --- 
                  +++ 
                  @@ -3,5 +3,4 @@
                   bla
                   bla
                   bla
                  -
                   ### END zone SALT ###

state.highstate (run3)

----------
          ID: uw.service.odyssey.01.feed file
    Function: file.managed
        Name: /test
      Result: True
     Comment: File /test exists with proper permissions. No changes made.
     Started: 12:51:23.545070
    Duration: 0.81 ms
     Changes:   
----------
          ID: uw.service.odyssey.01.feed block
    Function: file.blockreplace
        Name: /test
      Result: True
     Comment: No changes needed to be made
     Started: 12:51:23.545963
    Duration: 0.616 ms
     Changes:   
----------
@steverweber

This comment has been minimized.

Copy link
Contributor

commented May 4, 2016

note previous message had wrong version.. updated to be:

odyssey@odyssey-feed-1:/$ salt-call --version
salt-call 2015.5.9 (Lithium)
@steverweber

This comment has been minimized.

Copy link
Contributor

commented May 4, 2016

repeated same sls file on

root@odyssey-db-1:~# salt-call --version
salt-call 2015.8.8 (Beryllium)

same outcome.

@thatch45

This comment has been minimized.

Copy link
Member

commented May 4, 2016

This looks like you are inserting the newline as part of the content line:

content: {{ ('#{0}\n'.format(sls) + vars.contents)|json }}O

if you run state.highstate and look at the compiled data set is the newline in there?

@steverweber

This comment has been minimized.

Copy link
Contributor

commented May 4, 2016

content: {{ ('#{0}\n'.format(sls) + vars.contents)|json }}

that justs adds the value of sls to the block and appends the content...

I get the same issue without being fancy:

{{ sls }} file:
    file.managed:
        - name: /test
        - mode: 0400

{{ sls }} block:
    file.blockreplace:
        - name: /test
        - marker_start: "### START zone SALT ###"
        - marker_end: "### END zone SALT ###"
        - content: |
            bla
            bla
            bla
        - append_if_not_found: True
        - show_changes: True

perhaps my line endings are different in my editor.

@thatch45

This comment has been minimized.

Copy link
Member

commented May 4, 2016

I am just curious about what the compiled highdata looks like, just for that stanza, having that would help me reproduce it as well since I would know exactly what is going into the compiler

@steverweber

This comment has been minimized.

Copy link
Contributor

commented May 4, 2016

my EOL mode is UNIX...

whats the command to give the compiled highdata... state.low_state or somthing.?

@thatch45

This comment has been minimized.

Copy link
Member

commented May 4, 2016

state.show_highstate

@steverweber

This comment has been minimized.

Copy link
Contributor

commented May 4, 2016

hit another bug ...

root@odyssey-feed-1:~# salt-call --version
salt-call 2015.5.9 (Lithium)

root@odyssey-feed-1:~# salt-call state.show_highstate
[ERROR   ] An un-handled exception was caught by salt's global exception handler:                                                                                      
ValueError: '' is not in list                                                                                                                                          
Traceback (most recent call last):
  File "/usr/bin/salt-call", line 11, in <module>
    salt_call()
  File "/usr/lib/python2.7/dist-packages/salt/scripts.py", line 227, in salt_call
    client.run()
  File "/usr/lib/python2.7/dist-packages/salt/cli/call.py", line 61, in run
    caller = salt.cli.caller.Caller.factory(self.config)
  File "/usr/lib/python2.7/dist-packages/salt/cli/caller.py", line 69, in factory
    return ZeroMQCaller(opts, **kwargs)
  File "/usr/lib/python2.7/dist-packages/salt/cli/caller.py", line 92, in __init__
    self.minion = salt.minion.SMinion(opts)
  File "/usr/lib/python2.7/dist-packages/salt/minion.py", line 324, in __init__
    self.gen_modules(initial_load=True)
  File "/usr/lib/python2.7/dist-packages/salt/minion.py", line 339, in gen_modules
    self.utils = salt.loader.utils(self.opts)
  File "/usr/lib/python2.7/dist-packages/salt/loader.py", line 241, in utils
    whitelist=whitelist,
  File "/usr/lib/python2.7/dist-packages/salt/loader.py", line 739, in __init__
    self.refresh_file_mapping()
  File "/usr/lib/python2.7/dist-packages/salt/loader.py", line 859, in refresh_file_mapping
    if suffix_order.index(ext) < suffix_order.index(curr_ext):
ValueError: '' is not in list
@steverweber

This comment has been minimized.

Copy link
Contributor

commented May 4, 2016

anyways.

[root@salt-master-0 ~]# salt odyssey-db-1\* state.highstate test=true
odyssey-db-1.privatexxxxx:
----------
          ID: uw.service.odyssey.01.db file
    Function: file.managed
        Name: /test
      Result: None
     Comment: The file /test is set to be changed
     Started: 13:20:46.364776
    Duration: 16.655 ms
     Changes:   
              ----------
              newfile:
                  /test
----------
          ID: uw.service.odyssey.01.db block
    Function: file.blockreplace
        Name: /test
      Result: False
     Comment: /test: file not found
     Started: 13:20:46.381697
    Duration: 1.229 ms
     Changes:   

Summary
------------
Succeeded: 1 (unchanged=1, changed=1)
Failed:    1
root@odyssey-db-1:~# salt-call state.show_highstate
[INFO    ] Determining pillar cache
[INFO    ] Determining pillar cache
[INFO    ] Loading fresh modules for state activity
[INFO    ] Fetching file from saltenv 'base', ** skipped ** latest already in cache u'salt://top.sls'
[INFO    ] Fetching file from saltenv 'base', ** skipped ** latest already in cache u'salt://uw/service/odyssey/01/db/init.sls'
local:
    ----------
    uw.service.odyssey.01.db block:
        ----------
        __env__:
            base
        __sls__:
            uw.service.odyssey.01.db
        file:
            |_
              ----------
              name:
                  /test
            |_
              ----------
              marker_start:
                  ### START zone SALT ###
            |_
              ----------
              marker_end:
                  ### END zone SALT ###
            |_
              ----------
              content:
                  bla
                  bla
                  bla
            |_
              ----------
              append_if_not_found:
                  True
            |_
              ----------
              show_changes:
                  True
            - blockreplace
            |_
              ----------
              order:
                  10001
    uw.service.odyssey.01.db file:
        ----------
        __env__:
            base
        __sls__:
            uw.service.odyssey.01.db
        file:
            |_
              ----------
              name:
                  /test
            |_
              ----------
              mode:
                  400
            - managed
            |_
              ----------
              order:
                  10000
[root@salt-master-0 ~]# salt odyssey-db-1\* state.highstate
odyssey-db-1.private.uwaterloo.ca:
----------
          ID: uw.service.odyssey.01.db file
    Function: file.managed
        Name: /test
      Result: True
     Comment: Empty file
     Started: 13:22:04.191882
    Duration: 14.98 ms
     Changes:   
              ----------
              mode:
                  0400
              new:
                  file /test created
----------
          ID: uw.service.odyssey.01.db block
    Function: file.blockreplace
        Name: /test
      Result: True
     Comment: Changes were made
     Started: 13:22:04.207124
    Duration: 4.842 ms
     Changes:   
              ----------
              diff:
                  --- 
                  +++ 
                  @@ -0,0 +1,3 @@
                  +### START zone SALT ###
                  +bla
                  bla
                  bla

                  +### END zone SALT ###

Summary
------------
Succeeded: 2 (changed=2)
Failed:    0
------------
Total states run:     2
root@odyssey-db-1:~# salt-call state.show_highstate
[INFO    ] Determining pillar cache
[INFO    ] Determining pillar cache
[INFO    ] Loading fresh modules for state activity
[INFO    ] Fetching file from saltenv 'base', ** skipped ** latest already in cache u'salt://top.sls'
[INFO    ] Fetching file from saltenv 'base', ** skipped ** latest already in cache u'salt://uw/service/odyssey/01/db/init.sls'
local:
    ----------
    uw.service.odyssey.01.db block:
        ----------
        __env__:
            base
        __sls__:
            uw.service.odyssey.01.db
        file:
            |_
              ----------
              name:
                  /test
            |_
              ----------
              marker_start:
                  ### START zone SALT ###
            |_
              ----------
              marker_end:
                  ### END zone SALT ###
            |_
              ----------
              content:
                  bla
                  bla
                  bla
            |_
              ----------
              append_if_not_found:
                  True
            |_
              ----------
              show_changes:
                  True
            - blockreplace
            |_
              ----------
              order:
                  10001
    uw.service.odyssey.01.db file:
        ----------
        __env__:
            base
        __sls__:
            uw.service.odyssey.01.db
        file:
            |_
              ----------
              name:
                  /test
            |_
              ----------
              mode:
                  400
            - managed
            |_
              ----------
              order:
                  10000
@djneades

This comment has been minimized.

Copy link

commented May 4, 2016

Just to chime in, I am still seeing this problem (via salt-ssh) in Salt 2015.8.8.2. As @suslikas mentioned, it only happens on the first run, when the block is first added to a file that did not previously contain the boundary markers. The extra blank line is removed on subsequent runs.

@thatch45

This comment has been minimized.

Copy link
Member

commented May 4, 2016

What the heck? state.show_highstate does the same stuff that state.highstate does before running, and that is a suffix error in the loader, do you have an extra directory configured for your modules? or could there be a file in the loader directory that is tripping up the loader?

@thatch45

This comment has been minimized.

Copy link
Member

commented May 4, 2016

Thanks for the verify @dneades , I am still looking....

@steverweber

This comment has been minimized.

Copy link
Contributor

commented May 4, 2016

/ just ignore the show_highstate bug / yes i do some hacky things with the loader... but well it works in the newer version of salt so don't worry... time best spent on other things so i wont even report it :)

@thatch45

This comment has been minimized.

Copy link
Member

commented May 4, 2016

ok, thanks @steverweber !
I generally don't think about checking for all possible error states like that in the loader and we don't see them very often, if you can let me know what you are doing in the loader that differs I would appreciate it, if anything just for academic reasons :)

@steverweber

This comment has been minimized.

Copy link
Contributor

commented May 4, 2016

modules that are directories with __init__.py and inside that import a library local to the module path.

@thatch45

This comment has been minimized.

Copy link
Member

commented May 4, 2016

Ahh, got it, thanks!

@thatch45

This comment has been minimized.

Copy link
Member

commented May 4, 2016

Can haz fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.