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

Win lineinfile fix #35100

Merged
merged 4 commits into from
Feb 27, 2018
Merged

Win lineinfile fix #35100

merged 4 commits into from
Feb 27, 2018

Conversation

nwsparks
Copy link
Contributor

SUMMARY

Fixes #33858
Added test for this particular issue
Updated the copyright headers

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

win_lineinfile

ANSIBLE VERSION
ansible 2.5.0 (win_lineinfile_fix d5a530a351) last updated 2018/01/19 16:34:00 (GMT +000)
  config file = None
  configured module search path = [u'/root/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /opt/ansible/lib/ansible
  executable location = /opt/ansible/bin/ansible
  python version = 2.7.12 (default, Nov  2 2017, 19:20:38) [GCC 4.8.5 20150623 (Red Hat 4.8.5-11)]

ADDITIONAL INFORMATION

win_lineinfile had a section which was converting \r\n to`r`n` which is the PowerShell method of inserting line breaks. This was causing things like some windows paths (ex. c:\return\new) to be inserted incorrectly.

After removing the section, if line breaks are desired they can still be done path: "break\r\nhere". The quoting is important.

  - name: no line break
    win_lineinfile:
      path: c:\test.txt
      line: c:\return\new

  - name: no line break
    win_lineinfile:
      path: c:\test.txt
      line: 'c:\return\new'

  - name: line break occurs
    win_lineinfile:
      path: c:\test.txt
      line: "c:\return\new"

Any conversions are occurring in Ansible prior to being processed by PowerShell. So things like path: "break`r`nhere" are interpreted literally by powershell and no line break will occur.

@ansibot
Copy link
Contributor

ansibot commented Jan 19, 2018

@ansibot ansibot added affects_2.5 This issue/PR affects Ansible v2.5 bugfix_pull_request community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests. windows Windows community labels Jan 19, 2018
@sivel sivel removed the needs_triage Needs a first human triage before being processed. label Jan 22, 2018
@jborean93
Copy link
Contributor

The changes seem good to me but it would be good if you could add in

  • Some documentation/examples of how to create a regex with special characters like \t, e.g. PowerShell regex vs Python regex
  • A test for the above to prove that it does work

I know this will probably break people's playbooks but the changes you have here is arguably what they should be doing so once we have expanded the above I'm good to merge.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 6, 2018
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. community_review In order to be merged, this PR must follow the community review workflow. labels Feb 9, 2018
@mattclay
Copy link
Member

CI failure in integration tests: https://app.shippable.com/github/ansible/ansible/runs/54327/9/tests

@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Feb 13, 2018
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Feb 13, 2018
@nwsparks
Copy link
Contributor Author

Sorry for delays, been pretty busy at work. Added documentation and some additional testing per request. LMK if you want any changes.

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 13, 2018
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 21, 2018
@jborean93
Copy link
Contributor

@nwsparks sorry I hadn't looked at this earlier, looks good to me and I'll add to the 2.5 blocker list in case there is a new RC being released.

rebuild_merge

@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 27, 2018
@ansibot ansibot merged commit e15a903 into ansible:devel Feb 27, 2018
@jborean93 jborean93 added this to To Do in 2.5.x blocker list via automation Feb 27, 2018
@nwsparks
Copy link
Contributor Author

@jborean93 no problem, thanks for the review.

jborean93 pushed a commit to jborean93/ansible that referenced this pull request Mar 1, 2018
* win_lineinfile: fix ansible#33858. Removed conversion from \r\n

* win_lineinfile: added test for ansible#33858

* win_lineinfile: added documentation and more tests for change

* win_lineinfile: fixed wrong hash in testing

(cherry picked from commit e15a903)
@jborean93 jborean93 mentioned this pull request Mar 1, 2018
@jborean93
Copy link
Contributor

Backport PR for the stable-2.5 release is here #36888

@ansibot ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 6, 2018
nitzmahone pushed a commit that referenced this pull request Mar 6, 2018
* Win lineinfile fix (#35100)

* win_lineinfile: fix #33858. Removed conversion from \r\n

* win_lineinfile: added test for #33858

* win_lineinfile: added documentation and more tests for change

* win_lineinfile: fixed wrong hash in testing

(cherry picked from commit e15a903)

* Added changelog for win_lineinfile fix

* fix typo in changelog
@nitzmahone nitzmahone moved this from To Do to Done in 2.5.x blocker list Mar 8, 2018
@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.5 This issue/PR affects Ansible v2.5 bug This issue/PR relates to a bug. community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests. windows Windows community
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

win_lineinfile: windows paths have \r and \n stripped
5 participants