From cf69ec5db0f6666d29bd889ad227f92a2eac7b6f Mon Sep 17 00:00:00 2001 From: Evan Kaufman Date: Tue, 26 Mar 2019 08:49:58 -0700 Subject: [PATCH] replace - fixed combined before and after usage (#31452) When using before and after in combination, the opposite behavior was induced. This PR makes the the replacement happen between the specified patterns as intended. * Added integration tests * Add changelog, porting guide entry, and minor doc fixes --- .../fragments/replace-before-after.yaml | 2 + .../rst/porting_guides/porting_guide_2.7.rst | 2 + lib/ansible/modules/files/replace.py | 71 +++-- test/integration/targets/replace/aliases | 1 + .../integration/targets/replace/meta/main.yml | 2 + .../targets/replace/tasks/main.yml | 265 ++++++++++++++++++ 6 files changed, 317 insertions(+), 26 deletions(-) create mode 100644 changelogs/fragments/replace-before-after.yaml create mode 100644 test/integration/targets/replace/aliases create mode 100644 test/integration/targets/replace/meta/main.yml create mode 100644 test/integration/targets/replace/tasks/main.yml diff --git a/changelogs/fragments/replace-before-after.yaml b/changelogs/fragments/replace-before-after.yaml new file mode 100644 index 00000000000..a50d238932f --- /dev/null +++ b/changelogs/fragments/replace-before-after.yaml @@ -0,0 +1,2 @@ +bugfixes: + - replace - fix behavior when ``before`` and ``after`` are used together (https://github.com/ansible/ansible/issues/31354) diff --git a/docs/docsite/rst/porting_guides/porting_guide_2.7.rst b/docs/docsite/rst/porting_guides/porting_guide_2.7.rst index abd7a1b8ef8..ac412bb4fc9 100644 --- a/docs/docsite/rst/porting_guides/porting_guide_2.7.rst +++ b/docs/docsite/rst/porting_guides/porting_guide_2.7.rst @@ -224,6 +224,8 @@ Noteworthy module changes * The ``pip`` module has added a dependency on ``setuptools`` to support version requirements, this requirement is for the Python interpreter that executes the module and not the Python interpreter that the module is managing. +* Prior to Ansible 2.7.10, the ``replace`` module did the opposite of what was intended when using the ``before`` and ``after`` options together. This now works properly but may require changes to tasks. + Plugins ======= diff --git a/lib/ansible/modules/files/replace.py b/lib/ansible/modules/files/replace.py index c816d64de13..6c49a4d07da 100644 --- a/lib/ansible/modules/files/replace.py +++ b/lib/ansible/modules/files/replace.py @@ -57,21 +57,24 @@ options: - The string to replace regexp matches. - May contain backreferences that will get expanded with the regexp capture groups if the regexp matches. - If not set, matches are removed entirely. + - Backreferences can be used ambiguously like C(\1), or explicitly like C(\g<1>). type: str after: description: - - If specified, the line after the replace/remove will start. + - If specified, only content after this match will be replaced/removed. - Can be used in combination with C(before). - Uses Python regular expressions; see U(http://docs.python.org/2/library/re.html). + - Uses DOTALL, which means the C(.) special character I(can match newlines). type: str version_added: "2.4" before: description: - - If specified, the line before the replace/remove will occur. + - If specified, only content before this match will be replaced/removed. - Can be used in combination with C(after). - Uses Python regular expressions; see U(http://docs.python.org/2/library/re.html). + - Uses DOTALL, which means the C(.) special character I(can match newlines). type: str version_added: "2.4" backup: @@ -92,63 +95,78 @@ options: version_added: "2.4" notes: - As of Ansible 2.3, the I(dest) option has been changed to I(path) as default, but I(dest) still works as well. + - As of Ansible 2.7.10, the combined use of I(before) and I(after) works properly. If you were relying on the + previous incorrect behavior, you may be need to adjust your tasks. + See U(https://github.com/ansible/ansible/issues/31354) for details. - Option I(follow) has been removed in Ansible 2.5, because this module modifies the contents of the file so I(follow=no) doesn't make sense. ''' EXAMPLES = r''' -# Before Ansible 2.3, option 'dest', 'destfile' or 'name' was used instead of 'path' -- replace: +- name: Before Ansible 2.3, option 'dest', 'destfile' or 'name' was used instead of 'path' + replace: path: /etc/hosts regexp: '(\s+)old\.host\.name(\s+.*)?$' replace: '\1new.host.name\2' - backup: yes - name: Replace after the expression till the end of the file (requires Ansible >= 2.4) replace: - path: /etc/hosts - regexp: '(\s+)old\.host\.name(\s+.*)?$' - replace: '\1new.host.name\2' - after: Start after line.* - backup: yes + path: /etc/apache2/sites-available/default.conf + after: 'NameVirtualHost [*]' + regexp: '^(.+)$' + replace: '# \1' - name: Replace before the expression till the begin of the file (requires Ansible >= 2.4) replace: - path: /etc/hosts - regexp: '(\s+)old\.host\.name(\s+.*)?$' - replace: '\1new.host.name\2' - before: 'Start before line.*' - backup: yes + path: /etc/apache2/sites-available/default.conf + before: '# live site config' + regexp: '^(.+)$' + replace: '# \1' +# Prior to Ansible 2.7.10, using before and after in combination did the opposite of what was intended. +# see https://github.com/ansible/ansible/issues/31354 for details. - name: Replace between the expressions (requires Ansible >= 2.4) replace: path: /etc/hosts - regexp: '(\s+)old\.host\.name(\s+.*)?$' - replace: '\1new.host.name\2' - after: 'Start after line.*' - before: 'Start before line.*' - backup: yes + after: '' + before: '' + regexp: '^(.+)$' + replace: '# \1' -- replace: +- name: Supports common file attributes + replace: path: /home/jdoe/.ssh/known_hosts regexp: '^old\.host\.name[^\n]*\n' owner: jdoe group: jdoe mode: '0644' -- replace: +- name: Supports a validate command + replace: path: /etc/apache/ports regexp: '^(NameVirtualHost|Listen)\s+80\s*$' replace: '\1 127.0.0.1:8080' validate: '/usr/sbin/apache2ctl -f %s -t' - name: Short form task (in ansible 2+) necessitates backslash-escaped sequences - replace: dest=/etc/hosts regexp='\\b(localhost)(\\d*)\\b' replace='\\1\\2.localdomain\\2 \\1\\2' + replace: path=/etc/hosts regexp='\\b(localhost)(\\d*)\\b' replace='\\1\\2.localdomain\\2 \\1\\2' - name: Long form task does not replace: - dest: /etc/hosts + path: /etc/hosts regexp: '\b(localhost)(\d*)\b' replace: '\1\2.localdomain\2 \1\2' + +- name: Explicitly specifying positional matched groups in replacement + replace: + path: /etc/ssh/sshd_config + regexp: '^(ListenAddress[ ]+)[^\n]+$' + replace: '\g<1>0.0.0.0' + +- name: Explicitly specifying named matched groups + replace: + path: /etc/ssh/sshd_config + regexp: '^(?PListenAddress[ ]+)(?P[^\n]+)$' + replace: '#\g\g\n\g0.0.0.0' ''' import os @@ -231,7 +249,7 @@ def main(): pattern = u'' if params['after'] and params['before']: - pattern = u'%s(?P.*?)%s' % (params['before'], params['after']) + pattern = u'%s(?P.*?)%s' % (params['after'], params['before']) elif params['after']: pattern = u'%s(?P.*)' % params['after'] elif params['before']: @@ -242,6 +260,7 @@ def main(): match = re.search(section_re, contents) if match: section = match.group('subsection') + indices = [match.start('subsection'), match.end('subsection')] else: res_args['msg'] = 'Pattern for before/after params did not match the given file: %s' % pattern res_args['changed'] = False @@ -254,7 +273,7 @@ def main(): if result[1] > 0 and section != result[0]: if pattern: - result = (contents.replace(section, result[0]), result[1]) + result = (contents[:indices[0]] + result[0] + contents[indices[1]:], result[1]) msg = '%s replacements made' % result[1] changed = True if module._diff: diff --git a/test/integration/targets/replace/aliases b/test/integration/targets/replace/aliases new file mode 100644 index 00000000000..a6dafcf8cd8 --- /dev/null +++ b/test/integration/targets/replace/aliases @@ -0,0 +1 @@ +shippable/posix/group1 diff --git a/test/integration/targets/replace/meta/main.yml b/test/integration/targets/replace/meta/main.yml new file mode 100644 index 00000000000..07faa217762 --- /dev/null +++ b/test/integration/targets/replace/meta/main.yml @@ -0,0 +1,2 @@ +dependencies: + - prepare_tests diff --git a/test/integration/targets/replace/tasks/main.yml b/test/integration/targets/replace/tasks/main.yml new file mode 100644 index 00000000000..24146ff31b4 --- /dev/null +++ b/test/integration/targets/replace/tasks/main.yml @@ -0,0 +1,265 @@ +# setup +- set_fact: output_dir_test={{output_dir}}/test_replace + +- name: make sure our testing sub-directory does not exist + file: path="{{ output_dir_test }}" state=absent + +- name: create our testing sub-directory + file: path="{{ output_dir_test }}" state=directory + +# tests +- name: create test files + copy: + content: |- + The quick brown fox jumps over the lazy dog. + We promptly judged antique ivory buckles for the next prize. + Jinxed wizards pluck ivy from the big quilt. + Jaded zombies acted quaintly but kept driving their oxen forward. + dest: "{{ output_dir_test }}/pangrams.{{ item }}.txt" + with_sequence: start=0 end=6 format=%02x #increment as needed + + +## test `before` option +- name: remove all spaces before "quilt" + replace: + path: "{{ output_dir_test }}/pangrams.00.txt" + before: 'quilt' + regexp: ' ' + register: replace_test0 + +- command: "cat {{ output_dir_test }}/pangrams.00.txt" + register: replace_cat0 + +- name: validate before assertions + assert: + that: + - replace_test0 is successful + - replace_test0 is changed + - replace_cat0.stdout_lines[0] == 'Thequickbrownfoxjumpsoverthelazydog.' + - replace_cat0.stdout_lines[-1] == 'Jaded zombies acted quaintly but kept driving their oxen forward.' + + +## test `after` option +- name: remove all spaces after "promptly" + replace: + path: "{{ output_dir_test }}/pangrams.01.txt" + after: 'promptly' + regexp: ' ' + register: replace_test1 + +- command: "cat {{ output_dir_test }}/pangrams.01.txt" + register: replace_cat1 + +- name: validate after assertions + assert: + that: + - replace_test1 is successful + - replace_test1 is changed + - replace_cat1.stdout_lines[0] == 'The quick brown fox jumps over the lazy dog.' + - replace_cat1.stdout_lines[-1] == 'Jadedzombiesactedquaintlybutkeptdrivingtheiroxenforward.' + + +## test combined `before` and `after` options +- name: before "promptly" but after "quilt", replace every "e" with a "3" + replace: + path: "{{ output_dir_test }}/pangrams.02.txt" + before: 'promptly' + after: 'quilt' + regexp: 'e' + replace: '3' + register: replace_test2 + +- name: validate after+before assertions + assert: + that: + - replace_test2 is successful + - not replace_test2 is changed + - replace_test2.msg.startswith("Pattern for before/after params did not match the given file") + +- name: before "quilt" but after "promptly", replace every "e" with a "3" + replace: + path: "{{ output_dir_test }}/pangrams.03.txt" + before: 'quilt' + after: 'promptly' + regexp: 'e' + replace: '3' + register: replace_test3 + +- command: "cat {{ output_dir_test }}/pangrams.03.txt" + register: replace_cat3 + +- name: validate before+after assertions + assert: + that: + - replace_test3 is successful + - replace_test3 is changed + - replace_cat3.stdout_lines[1] == 'We promptly judg3d antiqu3 ivory buckl3s for th3 n3xt priz3.' + + +## test ^$ behavior in MULTILINE, and . behavior in absense of DOTALL +- name: quote everything between bof and eof + replace: + path: "{{ output_dir_test }}/pangrams.04.txt" + regexp: ^([\S\s]+)$ + replace: '"\1"' + register: replace_test4_0 + +- command: "cat {{ output_dir_test }}/pangrams.04.txt" + register: replace_cat4_0 + +- name: quote everything between bol and eol + replace: + path: "{{ output_dir_test }}/pangrams.04.txt" + regexp: ^(.+)$ + replace: '"\1"' + register: replace_test4_1 + +- command: "cat {{ output_dir_test }}/pangrams.04.txt" + register: replace_cat4_1 + +- name: validate before+after assertions + assert: + that: + - replace_test4_0 is successful + - replace_test4_0 is changed + - replace_test4_1 is successful + - replace_test4_1 is changed + - replace_cat4_0.stdout_lines[0] == '"The quick brown fox jumps over the lazy dog.' + - replace_cat4_0.stdout_lines[-1] == 'Jaded zombies acted quaintly but kept driving their oxen forward."' + - replace_cat4_1.stdout_lines[0] == '""The quick brown fox jumps over the lazy dog."' + - replace_cat4_1.stdout_lines[-1] == '"Jaded zombies acted quaintly but kept driving their oxen forward.""' + + +## test \b escaping in short and long form +- name: short form with unescaped word boundaries + replace: path="{{ output_dir_test }}/pangrams.05.txt" regexp='\b(.+)\b' replace='"\1"' + register: replace_test5_0 + +- name: short form with escaped word boundaries + replace: path="{{ output_dir_test }}/pangrams.05.txt" regexp='\\b(.+)\\b' replace='"\1"' + register: replace_test5_1 + +- command: "cat {{ output_dir_test }}/pangrams.05.txt" + register: replace_cat5_1 + +- name: long form with unescaped word boundaries + replace: + path: "{{ output_dir_test }}/pangrams.05.txt" + regexp: '\b(.+)\b' + replace: '"\1"' + register: replace_test5_2 + +- command: "cat {{ output_dir_test }}/pangrams.05.txt" + register: replace_cat5_2 + +- name: long form with escaped word boundaries + replace: + path: "{{ output_dir_test }}/pangrams.05.txt" + regexp: '\\b(.+)\\b' + replace: '"\1"' + register: replace_test5_3 + +- name: validate before+after assertions + assert: + that: + - not replace_test5_0 is changed + - replace_test5_1 is changed + - replace_test5_2 is changed + - not replace_test5_3 is changed + - replace_cat5_1.stdout_lines[0] == '"The quick brown fox jumps over the lazy dog".' + - replace_cat5_1.stdout_lines[-1] == '"Jaded zombies acted quaintly but kept driving their oxen forward".' + - replace_cat5_2.stdout_lines[0] == '""The quick brown fox jumps over the lazy dog"".' + - replace_cat5_2.stdout_lines[-1] == '""Jaded zombies acted quaintly but kept driving their oxen forward"".' + + +## test backup behaviors +- name: replacement with backup + replace: + path: "{{ output_dir_test }}/pangrams.06.txt" + regexp: ^(.+)$ + replace: '"\1"' + backup: true + register: replace_test6 + +- command: "cat {{ output_dir_test }}/pangrams.06.txt" + register: replace_cat6_0 + +- command: "cat {{ replace_test6.backup_file }}" + register: replace_cat6_1 + +- name: validate backup + assert: + that: + - replace_test6 is successful + - replace_test6 is changed + - replace_test6.backup_file is search('/pangrams.06.txt.') + - replace_cat6_0.stdout != replace_cat6_1.stdout + + +## test filesystem failures +- name: fail on directory + replace: + path: "{{ output_dir_test }}" + regexp: ^(.+)$ + register: replace_test7_1 + ignore_errors: true + +- name: fail on missing file + replace: + path: "{{ output_dir_test }}/missing_file.txt" + regexp: ^(.+)$ + register: replace_test7_2 + ignore_errors: true + +- name: validate backup + assert: + that: + - replace_test7_1 is failure + - replace_test7_2 is failure + - replace_test7_1.msg.endswith(" is a directory !") + - replace_test7_2.msg.endswith(" does not exist !") + + +## test subsection replacement when before/after potentially match more than once +- name: test file for subsection replacement gone awry + copy: + content: |- + # start of group + 0.0.0.0 + 127.0.0.1 + 127.0.1.1 + # end of group + + # start of group + 0.0.0.0 + 127.0.0.1 + 127.0.1.1 + # end of group + + # start of group + 0.0.0.0 + 127.0.0.1 + 127.0.1.1 + # end of group + dest: "{{ output_dir_test }}/addresses.txt" + +- name: subsection madness + replace: + path: "{{ output_dir_test }}/addresses.txt" + after: '# start of group' + before: '# end of group' + regexp: '0' + replace: '9' + register: replace_test8 + +- command: "cat {{ output_dir_test }}/addresses.txt" + register: replace_cat8 + +- name: validate before+after assertions + assert: + that: + - replace_test8 is successful + - replace_test8 is changed + - replace_cat8.stdout_lines[1] == "9.9.9.9" + - replace_cat8.stdout_lines[7] == "0.0.0.0" + - replace_cat8.stdout_lines[13] == "0.0.0.0"