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

fixed precedence on ansible.cfg #6038

Merged

Conversation

gamuniz
Copy link
Contributor

@gamuniz gamuniz commented Feb 23, 2020

SUMMARY

Fix to correctly use project directory before /etc/ansible/ansible.cfg in instances where a setting exists in both.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME
  • API
AWX VERSION
make VERSION
awx: 9.2.0
ADDITIONAL INFORMATION

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@AlanCoding
Copy link
Member

Based on the description of the issue and what you're doing here, this seems like it's probably what we want. But I want to get proper testing for it. I hope I can get around to that myself this afternoon tomorrow, unless someone else can first.

@@ -106,7 +106,7 @@ def could_be_inventory(project_path, dir_path, filename):
def read_ansible_config(project_path, variables_of_interest):
fnames = ['/etc/ansible/ansible.cfg']
if project_path:
fnames.insert(0, os.path.join(project_path, 'ansible.cfg'))
fnames.insert(1, os.path.join(project_path, 'ansible.cfg'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we just want fnames.append ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if it was done this way for another reason so I fixed it with as little changing as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, I'm just saying that based on what you're doing here, names.append(os.path.join(project_path, 'ansible.cfg')) is more succinct and accomplishes the same thing.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@AlanCoding
Copy link
Member

Sorry, still trying to get the testing straight.

@AlanCoding
Copy link
Member

This time I'm actually getting around to testing.

Copy link
Member

@AlanCoding AlanCoding left a comment

Choose a reason for hiding this comment

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

Found this quite hard to test, but eventually got it. The content for testing is at ansible/test-playbooks#124

make a JT that runs the debug playbook. Find that fails in devel, with role not found. Then use this branch, and find that it is successful.

@AlanCoding
Copy link
Member

This is about the millionth time I've said this, but hold off for a bit, still waiting for final test results.

@AlanCoding
Copy link
Member

Okay, now I'm good with merging this.

Copy link
Member

@kdelee kdelee left a comment

Choose a reason for hiding this comment

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

Alan's tests passed w/ this branch -- I say ship it

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit d08f592 into ansible:devel Mar 4, 2020
@gamuniz gamuniz deleted the honor_thy_precedence branch March 4, 2020 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants