Refactor initialize groups tasks used for byo #3895
Conversation
|
aos-ci-test |
There was a problem hiding this comment.
Great work here Russell.
Any thoughts on having a role create the groups instead of a playbook? It doesn't offer us much in terms of playbook vs role and code refactor other than it might be nice to encapsulate the work under a role. It feels like a function call.
Thoughts?
I will not block as I see this as awesome work! big 👍
| - name: Evaluate group l_oo_all_hosts | ||
| add_host: | ||
| name: "{{ item }}" | ||
| groups: l_oo_all_hosts |
There was a problem hiding this comment.
What's the significance of putting l_ on this variable? This is for my own sake. I've seen this pattern used else where in this repo.
I generally shy away from naming variables with l as it appears to the viewer as l, L, |, or even 1.
There was a problem hiding this comment.
local, something internal to the role that one should never expect to set from the outside
There was a problem hiding this comment.
@sdodson, thanks for the explanation. The irony of it is, if its internal to a role, then why are we in a playbook defining it? At that point it becomes global. Its fine for now but I'd prefer if we moved away from l_. I'd prefer group_ or something that denotes our intention.
There was a problem hiding this comment.
@kwoodson From what I've been able to dig out, the l_oo_all_hosts group was supposed to be something that was only referenced in the original playbook for creating the all g_ group names, which are then processed into the oo_ group names. The group name initialization process is something I have a card for to come back to. I've found where this l_oo_all_hosts group name has been used in other places, outside of the original playbook (probably by mistake), which should be corrected.
There was a problem hiding this comment.
@sdodson @mtnbikenc, Thanks for the explanations.
|
aos-ci-test |
|
[merge] |
|
[test]ing while waiting on the merge queue |
|
aos-ci-test |
|
flake openshift/origin#13271 |
|
aos-ci-test |
|
[merge] |
|
Flake openshift/origin#13108 |
|
Last test failed because of Ansible 2.3 being used for unit testing. Fixed in #3915. Trying again. [merge] |
|
@sdodson can you re-trigger the test? |
Two tasks for initializing group names for the byo playbooks was located in the common folder in the std_include.yml file. Byo dependencies should not be in the common folder. The two tasks have been removed from common/openshift-cluster/std_include.yml to a new file byo/openshift-cluster/initialize_groups.yml. All references where these tasks were included from either std_include.yml or other various files have been updated to use the byo initialize_groups.yml. The methodology implemented follows the pattern of having groups set up in byo then calling out to playbooks in common, which are common to all deployments.
mtnbikenc
force-pushed
the
playbook-refactor
branch
from
3af30f5
to
558796b
Compare
|
aos-ci-test |
|
Evaluated for openshift ansible merge up to 558796b |
|
Evaluated for openshift ansible test up to 558796b |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_openshift_ansible/24/) (Base Commit: 2942b03) |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_openshift_ansible/216/) (Base Commit: eb50698) |
Two tasks for initializing group names for the byo playbooks were located
in the common folder in the std_include.yml file. Byo dependencies
should not be in the common folder. The two tasks have been removed
from common/openshift-cluster/std_include.yml to a new file
byo/openshift-cluster/initialize_groups.yml. All references where these
tasks were included from either std_include.yml or other various files
have been updated to use the byo initialize_groups.yml. The methodology
implemented follows the pattern of having groups set up in byo then
calling out to playbooks in common, which are common to all deployments.
Entry point playbooks identified/modified in this change: