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

[WIP] lineinfile - Only insert line once when using firstmatch and insertafter #58946

Open
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@samdoran
Copy link
Member

commented Jul 10, 2019

SUMMARY

Fixes #58923

When using firstmatch and insertafter, the line is always inserted. There was no check to see if the line matched the line at the matched index value. I am not sure this ever worked correctly since we had no tests. I tested back to 2.5 and it always inserted multiple lines.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/modules/files/lineinfile.py

ADDITIONAL INFORMATION

Also remove the use of b() since using literal bytes strings is equivalent.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

@samdoran samdoran force-pushed the samdoran:issue/58923-lineinfile-firstmatch branch from 66f3983 to 1e0fa80 Jul 10, 2019

@mutt13y

This comment has been minimized.

Copy link

commented Jul 11, 2019

Hello,
I ran some tests and this works better but still not ideal and not according to my interpretation of the docs. However more importantly there is a practical issue which I will mention later.

First can I ask a question about how to test;
I cloned your fork and I ran hacking/env-setup but then it seems to fail because I do not have python2 modules installed. is it not possible to run with the pip3 modules ? What step did I miss?

What I ended up doing is copy lineinfile,py into my python3 ansible library after having backed up the one in there and run the tests with stock ansible 2.8.2

Now to the tests

  • line does not exist
    • it is added in the correct location (PASS)
  • line exists and is in the correct location
    • no changes made (PASS) (FIXED)
  • line exists (matches regex) in place but is incorrect
    • a new line is added in the correct location (FAIL!) (1)
  • line exists elsewhere in file (matches regex) and is correct
    • a new line is added in the correct location (FAIL ?) (2)

(1)
This is an issue in my opinion mainly because if you consider the use case
I have a playbook which has previously added the line in the correct place. Now I want to make a change to the line so I edit my play book and make a change to the line and run the playbook.
I think most peoples expectation is that it would change the line inlace and not add a new line

(2)
This is slightly more dubious, however like (1) my understanding of the docs is that IF regexp matches then insertafter and insertbefore are ignored
The problem with 2 is that there maybe playbooks that exploit this functionality.
IMVHO the docs are right and regexp should override lineafter/linebefore
at the very least there should be about option to make this the functionality.
regexpfirst: true/false say.

@samdoran samdoran changed the title lineinfile - Only insert line once when using firstmatch and insertafter [WIP] lineinfile - Only insert line once when using firstmatch and insertafter Jul 11, 2019

@ansibot ansibot added the WIP label Jul 11, 2019

@bcoca bcoca removed the needs_triage label Jul 11, 2019

@Andersson007
Copy link
Contributor

left a comment

I examined the related code and found that

The doc, particularly, says

If regular expressions are passed to both regexp and
insertafter, insertafter is only honored if no match for regexp is found.

that IMO literally means:

When regular expressions are passed to both regexp and insertafter, then:
1. regexp was found -> ignore insertafter, replace the founded line
2. regexp was not found -> insert the line after 'insertafter' line

@samdoran , please, look at lines 304, 309 - they may break the loop before regexp could be found.

291     b_line = to_bytes(line, errors='surrogate_or_strict')
292     for lineno, b_cur_line in enumerate(b_lines):
293         if regexp is not None:
294             match_found = bre_m.search(b_cur_line)
295         else:
296             match_found = b_line == b_cur_line.rstrip(b('\r\n'))
297         if match_found:
298             index[0] = lineno
299             m = match_found
300         elif bre_ins is not None and bre_ins.search(b_cur_line):
301             if insertafter:
302                 # + 1 for the next line
303                 index[1] = lineno + 1
304                 if firstmatch:
305                     break
306             if insertbefore:
307                 # index[1] for the previous line
308                 index[1] = lineno
309                 if firstmatch:
310                     break

Therefore the code or the documentation fragment is not really correct.

So we can:

  1. Change the documentation
  2. Change the incorrect behavior
    In addition to @samdoran 's changes, the solution may look logically (very roughly) like:
290     m = None
291     b_line = to_bytes(line, errors='surrogate_or_strict')
292 
293     # 1. First check that there is no match for regexp:
294     if regexp is not None:
295         for lineno, b_cur_line in enumerate(b_lines):
296             match_found = bre_m.search(b_cur_line)
297             if match_found:
298                 index[0] = lineno
299                 m = match_found
300                 if firstmatch:
301                     break
302 
303     # 2. When no match found, parse for searching insertafter/insertbefore:
304     if not m:
305         for lineno, b_cur_line in enumerate(b_lines):
306             if bre_ins.search(b_cur_line):
307                 if insertafter:
308                     # + 1 for the next line
309                     index[1] = lineno + 1
310                     if firstmatch:
311                         break
312 
313                 if insertbefore:
314                     # index[1] for the previous line
315                     index[1] = lineno
316                     if firstmatch:
317                         break
318 

(^^ it's only logically, regarding that documentation says, I don't insist on this quick solution, of course, better to implement it by one loop but, at least, it doesn't break the current existent CI tests :)
Hope my small research will be useful to solve the issue, guys

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.