-
Notifications
You must be signed in to change notification settings - Fork 656
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
fix(provisioner): Expand ANSIBLE_ variables with Ansible default locations #2258
Conversation
…tions Signed-off-by: wilmardo <info@wilmardenouden.nl>
@@ -202,11 +202,11 @@ class Ansible(base.Base): | |||
:: | |||
|
|||
ANSIBLE_ROLES_PATH: | |||
$ephemeral_directory/roles/:$project_directory/../ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The library -> modules and filters -> filter path changes are breaking changes or?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but only if a user has overridden these variables in the molecule.yml. Otherwise the new paths are correctly expanded by the code change in ansible.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it shouldn't be that much of an issue since the documentation was already wrong. The path was $ephemeral_directory/libraries
in the code but $ephemeral_directory/library
in the docs. If have not seen any issues about it. Also the library only contains the internal vagrant module.
The filters plugin could break by the point above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhhhh, right. Well, I'm not sure what to do then. We should perhaps include a more clarifying note in the change log that this could break things for end-users? @ssbarnea what do you think here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick pass, makes sense to me, in general.
First, be sure it passes CI, after this I will have a second look because this change has a lot of potential,.... of breaking stuff. I do understand reasoning behind and support them (to avoid diverging from ansible default behavior), but I need to be sure we do this right or we will all pay the costs of breaking things. |
Signed-off-by: wilmardo <info@wilmardenouden.nl>
Signed-off-by: wilmardo <info@wilmardenouden.nl>
Signed-off-by: wilmardo <info@wilmardenouden.nl>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see that someone else has validated this with manual test logs before we merge.
Signed-off-by: wilmardo <info@wilmardenouden.nl>
@ssbarnea Fixed formatting :) @decentral1se Someone in mind? I could poke on of my colleagues to test this? |
That would be fantastic. If not, well, we can merge and rely on early Q&A testing too. |
Will do tomorrow! |
PR Type
Changes
For more information about local modules and plugins see the Ansible documentation:
https://docs.ansible.com/ansible/latest/dev_guide/developing_locally.html#adding-a-module-locally