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

Ensure _raw_params retain exact spaces #45853

Merged
merged 4 commits into from Sep 28, 2018

Conversation

dagwieers
Copy link
Contributor

@dagwieers dagwieers commented Sep 19, 2018

SUMMARY

This fixes a longstanding bug related to multi-line YAML strings that lack the original whitespaces and gain a few new whitespaces after newlines.

ISSUE TYPE
  • Bugfix Pull Request
  • Docs Pull Request
COMPONENT NAME

shell (and others)

ANSIBLE VERSION

v2.8 and older

ADDITIONAL INFORMATION

This PR fixes issues with input handling. If you would do the following:

- hosts: localhost
  tasks:
  - shell: |
      ls
      cd
      echo "Foo"

This would translate to: ls\n cd\n echo "Foo"

Any indentation would get lost, all prefixed and trailing whitespaces are eaten and everything is joined with whitespaces (so one whitespace was always added after a newline).

This breaks various things link indentation, but most importantly here-documents in the shell module.

- hosts: localhost
  tasks:
  - shell: |
      cat <<EOF
        One
        Two
          Three
        Four
      EOF

This would translate into something that fails to work as designed:

cat <<EOF
 One
 Two
 Three
 Four
 EOF

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Sep 19, 2018
@dagwieers dagwieers force-pushed the pure_raw_params branch 6 times, most recently from 759e5af to 4b38ed8 Compare September 19, 2018 16:56
@dagwieers
Copy link
Contributor Author

Given the impact (this has bothered me for years) we may want to back-port this.

@dagwieers dagwieers force-pushed the pure_raw_params branch 3 times, most recently from 26231b2 to 1483e25 Compare September 19, 2018 17:08
@jborean93 jborean93 removed the needs_triage Needs a first human triage before being processed. label Sep 20, 2018
@alikins
Copy link
Contributor

alikins commented Sep 20, 2018

tests?

@dagwieers
Copy link
Contributor Author

@alikins Sure, but apparently there's no consensus that we want to fix this.

bcoca
bcoca previously requested changes Sep 20, 2018
Copy link
Member

@bcoca bcoca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks fine, but i would add tests for this use case. Specially since string manipulation is very finicky across python versions.

Also i would leave the note about using script module, it just keeps playbooks more readable.

@abadger
Copy link
Contributor

abadger commented Sep 20, 2018

At today's meeting no one objected to this and several people expressed interest. However, the backwards compatibility was a question with mitigation for that being to have a config toggle and deprecation period. Both bcoca and I wanted the documentation to continue recommending script. Two reasons for that would be that it keeps playbooks more readable and the parsing code seems to be a never ending source of cornercases as it has to make decisions about reformatting which may not always be obvious or the way people want it. dag noted that he wasn't sure if docs should advise people to do something else as it might be that the usage fit their use case fine.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Sep 20, 2018
@dagwieers
Copy link
Contributor Author

@bcoca @abadger Do you think backward compatibility breaks because of correct spacing ? I have tried to wrap my head around it, but I don't see how this could fail for existing use-case. Either the incorrect spacing was an issue (and people could not use the shell module) or it did work and correct spacing was irrelevant (and this has no real impact).

Can we maybe add it like this and see of people report any issues before adding a flag and deprecation ?

@bcoca
Copy link
Member

bcoca commented Sep 21, 2018

I don't know about any case that it breaks existing playbooks, I would be surprised anyone found the current shell + multiline useful .. but who knows? people do weird stuff.

I would add to devel and let it macerate for a while, then consider backports.

This fixes a few issues related to multi-line YAML strings that eat
whitespace and add whitespaces after newlines (that weren't there).
By checking the cmd result with the original we cause these tests to
fail on older releases without this PR.
@ansibot ansibot added the support:community This issue/PR relates to code supported by the Ansible community. label Sep 21, 2018
@dagwieers
Copy link
Contributor Author

I think I took in all feedback, please re-review.

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Sep 24, 2018
@dagwieers dagwieers dismissed bcoca’s stale review September 26, 2018 11:10

I think I incorporated all feedback, can you re-review ?

@dagwieers
Copy link
Contributor Author

rebuild_merge

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Sep 27, 2018
@dagwieers dagwieers merged commit ff06320 into ansible:devel Sep 28, 2018
@mattclay
Copy link
Member

mattclay commented Oct 2, 2018

These changes result in a traceback for some inputs, see #46379 for details.

pilou- added a commit to pilou-/ansible that referenced this pull request Dec 17, 2018
ensure whitespace isn't added after a newline (see ansible#45853)
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants