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

Don't skip an inventory source just because it has a comma #35002

Merged
merged 2 commits into from
Jan 24, 2018
Merged

Don't skip an inventory source just because it has a comma #35002

merged 2 commits into from
Jan 24, 2018

Conversation

sivel
Copy link
Member

@sivel sivel commented Jan 17, 2018

SUMMARY

Don't skip an inventory source just because it has a comma, make sure it's also doesn't exist as a path. Fixes #34931

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/vars/manager.py

ANSIBLE VERSION
devel
2.4
ADDITIONAL INFORMATION

@sivel sivel mentioned this pull request Jan 17, 2018
Closed
@ansibot ansibot added affects_2.5 This issue/PR affects Ansible v2.5 bugfix_pull_request 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 Jan 17, 2018
@ansibot
Copy link
Contributor

ansibot commented Jan 17, 2018

The test ansible-test sanity --test validate-modules [?] failed with the following errors:

lib/ansible/modules/cloud/amazon/cloudfront_distribution.py:0:0: E322 "access_key" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/cloudfront_distribution.py:0:0: E322 "access_token" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/cloudfront_distribution.py:0:0: E322 "aws_access_key" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/cloudfront_distribution.py:0:0: E322 "aws_region" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/cloudfront_distribution.py:0:0: E322 "aws_secret_key" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/cloudfront_distribution.py:0:0: E322 "ec2_access_key" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/cloudfront_distribution.py:0:0: E322 "ec2_region" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/cloudfront_distribution.py:0:0: E322 "ec2_secret_key" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/cloudfront_distribution.py:0:0: E322 "ec2_url" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/cloudfront_distribution.py:0:0: E322 "profile" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/cloudfront_distribution.py:0:0: E322 "purge_aliases" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/cloudfront_distribution.py:0:0: E322 "region" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/cloudfront_distribution.py:0:0: E322 "secret_key" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/cloudfront_distribution.py:0:0: E322 "security_token" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/amazon/cloudfront_distribution.py:0:0: E322 "validate_certs" is listed in the argument_spec, but not documented in the module

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 Jan 17, 2018
@abadger
Copy link
Contributor

abadger commented Jan 17, 2018

There's an inherent collision here between valid path names -i "./Web,database/hosts.yml" and specifying hosts on the command line -i web.lan,database.lan. I'm not sure how far we can change this to satisfy one of these without causing difficulties for the other. And adding more heuristics might just serve to make this magical, just-works-except-when-it doesn't behaviour. Other than being slow, we could just as validly be checking whether the string was a valid hostname as whether it was a valid path. And what if it happened to be both a valid hostname and path?

So I'm not against this getting into devel but I think we're going to have further problems in the future (likely from the other direction....) I think we should create separate syntax or cli switches for inventory files versus hosts to add on the cli in the future and deprecate putting both values in the same switch. (-i @hostfile,new_host )? But changing/removing CLI switches is a long term thing that we probably have to give users more than a normal deprecation cycle to get used to. So maybe this is a good enough change for now?

(What if we demand that there's "/" in any inventory path that also has a comma? That would solve the existing reported issue.)

@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jan 17, 2018
@sivel sivel removed the needs_triage Needs a first human triage before being processed. label Jan 17, 2018
@sivel sivel mentioned this pull request Jan 18, 2018
Closed
@sivel
Copy link
Member Author

sivel commented Jan 23, 2018

The decision was made in the 2018-01-23 core meeting to move forward with merging this to devel and to temp-staging-post-2.4.3 for a potential inclusion for a 2.4.4 release

@ansibot ansibot added support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests. labels Jan 23, 2018
@sivel sivel merged commit 1ac04ba into ansible:devel Jan 24, 2018
sivel added a commit that referenced this pull request Jan 24, 2018
@sivel
Don't skip an inventory source just because it has a comma (#35002)
e240166 
* Don't skip an inventory source just because it has a comma, make sure it's also doesn't exist as a path. Fixes #34931

* Add integration test for inventory path with commas

(cherry picked from commit 1ac04ba)
abadger pushed a commit that referenced this pull request Feb 1, 2018
@sivel @abadger
Don't skip an inventory source just because it has a comma (#35002)
2924ad1 
* Don't skip an inventory source just because it has a comma, make sure it's also doesn't exist as a path. Fixes #34931

* Add integration test for inventory path with commas

(cherry picked from commit 1ac04ba)
Lujeni pushed a commit to Lujeni/ansible that referenced this pull request Feb 1, 2018
@sivel @Lujeni
Don't skip an inventory source just because it has a comma (ansible#3…
884cf70 
…5002)

* Don't skip an inventory source just because it has a comma, make sure it's also doesn't exist as a path. Fixes ansible#34931

* Add integration test for inventory path with commas
@ansibot ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 6, 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.
Reviewers

@nitzmahone nitzmahone nitzmahone approved these changes

Assignees
No one assigned
Labels
affects_2.5 This issue/PR affects Ansible v2.5 bug This issue/PR relates to a bug. 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
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error when directory name is comma and space
4 participants
@sivel @ansibot @abadger @nitzmahone