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: windows paths have \r and \n stripped #33858

Closed
dannietjoh opened this issue Dec 13, 2017 · 6 comments · Fixed by #35100
Closed

win_lineinfile: windows paths have \r and \n stripped #33858

dannietjoh opened this issue Dec 13, 2017 · 6 comments · Fixed by #35100
Labels
affects_2.4 This issue/PR affects Ansible v2.4 bug This issue/PR relates to a bug. module This issue/PR relates to a module. support:community This issue/PR relates to code supported by the Ansible community. windows Windows community

Comments

@dannietjoh
Copy link

ISSUE TYPE

Bug Report

COMPONENT NAME

win_lineinfile

ANSIBLE VERSION
ansible 2.4.1.0
  config file = None
  configured module search path = [u'/none/of/your/business', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/local/lib/python2.7/dist-packages/ansible
  executable location = /usr/local/bin/ansible
  python version = 2.7.14 (default, Sep 23 2017, 22:06:14) [GCC 7.2.0]
CONFIGURATION
OS / ENVIRONMENT
SUMMARY

this:

$line = $line.Replace("\r", "`r");

butchers any windows path and also makes regexes fail and destroys idempotency

STEPS TO REPRODUCE

use win_lineinefile to add a windows path containing \r or \n

- win_lineinfile:
    path: C:\temp\testfile.txt
    line: 'C:\path\returning\from\nowhere'

OR

- win_lineinfile:
    path: C:\temp\testfile.txt
    line: "C:\\path\\returning\\from\\nowhere"
EXPECTED RESULTS

file C:\temp\testfile.tx contains C:\path\returning\from\nowhere

ACTUAL RESULTS

file C:\temp\testfile.tx contains C:\patheturning\fromowhere

Current workaround is to use forward slashes, but that requires forward slash compatibility in any program that uses this file

@dannietjoh dannietjoh changed the title windows paths have \r and \n stripped win_lineinfile: windows paths have \r and \n stripped Dec 13, 2017
@ansibot
Copy link
Contributor

ansibot commented Dec 13, 2017

Files identified in the description:

If these files are inaccurate, please update the component name section of the description or use the !component bot command.

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Dec 13, 2017

@ansibot ansibot added affects_2.4 This issue/PR affects Ansible v2.4 bug_report 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. windows Windows community labels Dec 13, 2017
@gundalow gundalow removed the needs_triage Needs a first human triage before being processed. label Dec 13, 2017
@ansibot
Copy link
Contributor

ansibot commented Jan 6, 2018

Files identified in the description:

If these files are inaccurate, please update the component name section of the description or use the !component bot command.

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Jan 6, 2018

@ansibot
Copy link
Contributor

ansibot commented Jan 6, 2018

Files identified in the description:

If these files are inaccurate, please update the component name section of the description or use the !component bot command.

click here for bot help

@nwsparks
Copy link
Contributor

I think there are two ways of approaching this.

  1. Always treat line as literal. Multiple lines don't fit into the purpose of the module. I did test the Linux version of this module however, and it does allow line breaks....so this may not be an acceptable answer.
  2. Remove this validation block. If people want to insert line breaks they should do it as a PowerShell native/acceptable format.

If ($line) {
$line = $line.Replace("\r", "`r");
$line = $line.Replace("\n", "`n");
$line = $line.Replace("``r", "`r");
$line = $line.Replace("``n", "`n");
}

In addition, lines 350-351 don't make sense as they are converting what is an attempt to escape the line break and print it literally, into a line break. I can't see a good reason for doing this conversion.

I'll bring it up in the WWG today.

nwsparks added a commit to nwsparks/ansible that referenced this issue Jan 19, 2018
nwsparks added a commit to nwsparks/ansible that referenced this issue Jan 19, 2018
ansibot pushed a commit that referenced this issue Feb 27, 2018
* 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
jborean93 pushed a commit to jborean93/ansible that referenced this issue 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)
nitzmahone pushed a commit that referenced this issue 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
@ansibot ansibot added bug This issue/PR relates to a bug. and removed bug_report labels Mar 7, 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.4 This issue/PR affects Ansible v2.4 bug This issue/PR relates to a bug. module This issue/PR relates to a module. support:community This issue/PR relates to code supported by the Ansible community. windows Windows community
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants