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.

Sign up for GitHub

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

Jump to bottom

Add better error when k=v syntax is used with YAML in tasks #41754

Merged
merged 7 commits into from
Dec 4, 2018
Merged

Add better error when k=v syntax is used with YAML in tasks #41754

merged 7 commits into from
Dec 4, 2018

Conversation

samdoran
Copy link
Contributor

SUMMARY

Detect and report an error when Ansible k=v syntax is use with a task formatted as YAML as well as when = are used in YAML tasks rather than :.

Add unit tests for new error message.

Examples of syntax that causes this error:

- user: name=bob state=present
    name: bob
    state: present

- user:
    name=bob
    state: present

Example output:

> ansible-playbook testlocal.yml
ERROR! Syntax Error while loading YAML.
  mapping values are not allowed in this context

The error appears to be in '/Users/sdoran/Projects/Vagrant/Ansible Lab/testlocal.yml': line 9, column 9, but may
be elsewhere in the file depending on the exact syntax problem.

The offending line appears to be:

    name=bob
        ^ here
    state: present

There appears to be both 'k=v' shorthand syntax and YAML in this task. Only one syntax may be used.

Fixes #27210

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/errors/__init__.py
lib/ansible/errors/yaml_strings.py

ANSIBLE VERSION
2.7

@samdoran samdoran force-pushed the issue/27210-kv-syntax-error branch from 9b9fdfd to fed98b2 Compare June 20, 2018 20:15
@ansibot ansibot added affects_2.7 This issue/PR affects Ansible v2.7 bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. labels Jun 20, 2018
@samdoran samdoran force-pushed the issue/27210-kv-syntax-error branch from fed98b2 to d57efba Compare June 20, 2018 20:18
@samdoran samdoran removed the needs_triage Needs a first human triage before being processed. label Jun 20, 2018
@ansibot
Copy link
Contributor

ansibot commented Jun 20, 2018

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/errors/yaml_strings.py:137:25: E221 multiple spaces before operator

click here for bot help

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jun 20, 2018
@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jun 30, 2018
@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 Aug 15, 2018
@gundalow
Copy link
Contributor

I like it, think we can get this into 2.7?

@samdoran
Copy link
Contributor Author

@gundalow We need to get some more reviewers to make sure I did this properly. Please ping some folks.

@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 Aug 28, 2018
@samdoran samdoran added this to To Do - Backlog only. Anything Can be here. in Ansible 2.8 Sep 25, 2018
@thaumos thaumos moved this from To Do - Backlog only. Anything Can be here. to Approved - This contains prioritised work that will block the release. It will contained assigned work. in Ansible 2.8 Oct 10, 2018
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html 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. labels Oct 16, 2018
@samdoran
Add error message for k=v and YAML in a single task
f9308d4 
Find the correct line, column, and position for k=v errors since they are different than the position reported initially.

Document bug in quoting syntax check.
@samdoran
Change tense or error message
38ed48f 
Since the error still exists, switch to present tense rather than past tense.
@samdoran
Remove double spaces after periods in error messages.
31c7569 
http://www.slate.com/articles/technology/technology/2011/01/space_invaders.html
@samdoran
Add changelog fragment
65d9030
@samdoran
Add tests for new error message
8e0d85e
@samdoran
Fix tests
fc07b62
@ansibot ansibot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Nov 5, 2018
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. 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. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Nov 5, 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 Nov 13, 2018
@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 Nov 16, 2018
@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 Nov 19, 2018
@abadger
Copy link
Contributor

abadger commented Nov 22, 2018

rebuild_merge

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Dec 4, 2018
@ansibot ansibot merged commit 40a5f7b into ansible:devel Dec 4, 2018
@samdoran samdoran deleted the issue/27210-kv-syntax-error branch December 5, 2018 16:50
kbreit pushed a commit to kbreit/ansible that referenced this pull request Jan 11, 2019
@samdoran @kbreit
Add better error when k=v syntax is used with YAML in tasks (ansible#…
b0a2924 
…41754)

* Add error message for k=v and YAML in a single task

Find the correct line, column, and position for k=v errors since they are different than the position reported initially.

Document bug in quoting syntax check.

* Change tense or error message

Since the error still exists, switch to present tense rather than past tense.

* Remove double spaces after periods in error messages.

http://www.slate.com/articles/technology/technology/2011/01/space_invaders.html

* Add changelog fragment

* Add tests for new error message

* Fix tests

* Add clarifying comments to unit test
@bcoca bcoca moved this from Approved - This contains prioritised work that will block the release. It will contained assigned work. to Done - This means done, tests, integration, everything... obvi. in Ansible 2.8 Jan 29, 2019
@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.
Reviewers

@gundalow gundalow gundalow approved these changes

Assignees
No one assigned
Labels
affects_2.7 This issue/PR affects Ansible v2.7 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. 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. test This PR relates to tests.
Projects
No open projects
Ansible 2.8
  
Done - This means done, tests, integr...
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

yaml error has misleading suggestion to fix escaping issue when mixing key=value args and yaml dict args
4 participants
@samdoran @ansibot @gundalow @abadger