-
-
Notifications
You must be signed in to change notification settings - Fork 75
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
Port to minimum of Ansible 2.5 (resolves #190) #215
Conversation
Replaces `with_*` loops with the new `loop` keyword.[1] Note that with_dict => loop uses the dictsort filter as dict2items is not available until Ansible 2.6. [1] https://docs.ansible.com/ansible/2.7/porting_guides/porting_guide_2.5.html#migrating-from-with-x-to-loop
Replaces static includes with import_tasks (partially resolves StackStorm#190).
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.
Thanks for the contribution!
Left a few cosmetic comments. Overall looks nice.
Need to check also not so obvious migration points like with_items
.
@@ -5,3 +5,6 @@ | |||
yum_resository: | |||
name: "StackStorm_{{ bwc_repo }}" | |||
state: absent | |||
tags: | |||
- BWC repos |
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.
Don't need BWC repos
tag
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.
These tags orignally came from:
- roles/bwc/tasks/bwc_repos_setup.yml:
tags
on "Handle bwc_license change"include: license.yml
- roles/bwc/tasks/license.yml: "Cleanup repo list file from disk"
Since include_tasks
can't propagate tags until a later version of ansible, I manually propagated the tags.
If you want me to drop the ewc tag, that's fine.
I did a general switch of BWC -> EWC including tags, task names, comments and some readme entries. I did not update variable names, file names, or role names to avoid breaking anyone using these roles. |
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.
Thanks for the contribution! Looks good 👍
Obviously 2.5
migration is a little bit more involved than we all thought :)
Will do a few more checks and merge it.
Replaces dynamic includes with include_tasks (resolves StackStorm#190). include_tasks (introduced in Ansible 2.4) does not support ansible 1.x style inline vars syntax (var=value) (since at least Ansible 2.4.1)[1]. If the syntax ever worked, removing support for it was announced in the Ansible 2.7 Porting Guide[2]. So, this modifies include loops to be include_tasks loops using the `vars` key instead of the inline syntax. [1] ansible/ansible#32812 [2] https://docs.ansible.com/ansible/2.7/porting_guides/porting_guide_2.7.html#include-tasks-import-tasks-inline-variables
Dynamic includes (include_tasks) do not apply tags to internal tasks.[1] Once we transition to 2.7, we can use `apply` on the `include_tasks` module to specify the tags for included files.[2] [1] https://docs.ansible.com/ansible/2.7/porting_guides/porting_guide_2.5.html#dynamic-includes-and-attribute-inheritance [2] https://docs.ansible.com/ansible/2.7/modules/include_tasks_module.html
Bump the minimum ansible version for the remaining roles.
With 2.5, facts are now namespaced under ansible_facts.[1] They are still accessible under the ansible_* vars, however that can be disabled by setting `inject_facts_as_vars` to False. The default is True in 2.5, but "is expected to be set to 'False' in a future release. Migrating now means these roles and playbooks do not depend on this setting being True. [1] https://docs.ansible.com/ansible/2.7/porting_guides/porting_guide_2.5.html#ansible-fact-namespacing
ec9cfd1
to
ad25be0
Compare
Port to Ansible 2.5 (resolves #190)
Thanks to @tzyychin for helping me put this together!