-
Notifications
You must be signed in to change notification settings - Fork 23.7k
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
Add warning when using an empty regexp in lineinfile #42013
Add warning when using an empty regexp in lineinfile #42013
Conversation
@@ -480,12 +480,17 @@ def main(): | |||
regexp = params['regexp'] | |||
line = params.get('line', None) | |||
|
|||
if regexp == '': | |||
module.warn( | |||
'The regular expression is empty, which will match every line in the file.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to say "empty string". We also may want to give an alternative for people who actually do want to match every line in the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "give an alternative"? This warning could be annoying to someone who does want this capability, and I don't want to encourage folks to disable warnings with noisy error messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Give an example of a regex to use if they really want to match every line. (I think "^" would do it and be efficient). We could also give an example of unsetting it but that isn't as succinctly written.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's perfect! I see what you're saying.
@@ -717,37 +717,3 @@ | |||
assert: | |||
that: | |||
- result.stat.checksum == 'eca8d8ea089d4ea57a3b87d4091599ca8b60dfd2' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping tests (but updating if the results have changed) seems like a good idea. Then it's an additional clue that the behaviour was meant to be this way.
Also, I think we want a changelog for this? (And maybe a porting guide?) |
5cd76fe
to
66e41e5
Compare
Offer an equivalent regexp that won't trigger the warning. Update tests to match new warning.
* Revert "Account for empty string regexp in lineinfile (ansible#41451)" This reverts commit 4b5b4a7. * Use context managers for interacting with files * Store line and regexp parameters in a variable * Add warning when regexp is an empty string * Remove '=' from error messages * Update warning message and add changelog * Add tests * Improve warning message Offer an equivalent regexp that won't trigger the warning. Update tests to match new warning. * Add porting guide entry for lineinfile change # Conflicts: # docs/docsite/rst/porting_guides/porting_guide_2.7.rst # lib/ansible/modules/files/lineinfile.py # test/integration/targets/lineinfile/tasks/main.yml
* Revert "Account for empty string regexp in lineinfile (ansible#41451)" This reverts commit 4b5b4a7. * Use context managers for interacting with files * Store line and regexp parameters in a variable * Add warning when regexp is an empty string * Remove '=' from error messages * Update warning message and add changelog * Add tests * Improve warning message Offer an equivalent regexp that won't trigger the warning. Update tests to match new warning. * Add porting guide entry for lineinfile change (cherry picked from commit fb55038)
* Revert "Account for empty string regexp in lineinfile (#41451)" This reverts commit 4b5b4a7. * Use context managers for interacting with files * Store line and regexp parameters in a variable * Add warning when regexp is an empty string * Remove '=' from error messages * Update warning message and add changelog * Add tests * Improve warning message Offer an equivalent regexp that won't trigger the warning. Update tests to match new warning. * Add porting guide entry for lineinfile change (cherry picked from commit fb55038)
* Add warning when using an empty regexp in lineinfile (#42013) * Revert "Account for empty string regexp in lineinfile (#41451)" This reverts commit 4b5b4a7. * Use context managers for interacting with files * Store line and regexp parameters in a variable * Add warning when regexp is an empty string * Remove '=' from error messages * Update warning message and add changelog * Add tests * Improve warning message Offer an equivalent regexp that won't trigger the warning. Update tests to match new warning. * Add porting guide entry for lineinfile change # Conflicts: # docs/docsite/rst/porting_guides/porting_guide_2.7.rst # lib/ansible/modules/files/lineinfile.py # test/integration/targets/lineinfile/tasks/main.yml * Add porting guide info
SUMMARY
Reverts #41451 and adds a warning instead.
See #29443 for reasoning behind this change.
ISSUE TYPE
COMPONENT NAME
lineinfile.py
ANSIBLE VERSION