polish openshift-master role #4947
Conversation
ingvagabund
force-pushed
the
polish-openshift-master-role
branch
from
f5653bb to
1e870f3
Compare
| @@ -57,39 +67,43 @@ | |||
| args: | |||
| creates: "{{ openshift_master_policy }}" | |||
| notify: | |||
| - restart master | |||
There was a problem hiding this comment.
Always confused by the indentation. This looks ugly to me but maybe it should be this way. Any justification why beside readability?
| failed_when: false | ||
| changed_when: false | ||
| register: already_set | ||
| - name: check whether our docker-registry setting exists in the env file |
There was a problem hiding this comment.
@tbielawa undoing your change. Even if the Ansible docs shows as correct, I don't like it.
[1] http://docs.ansible.com/ansible/latest/playbooks_blocks.html#id1
There was a problem hiding this comment.
No matter which style we choose I would like to have that documented here [1]. Including indentation in both block and notify.
[1] https://github.com/openshift/openshift-ansible/blob/master/docs/best_practices_guide.adoc
|
aos-ci-test |
|
Can you layer this after #4832 (which removes a significant chunk of code, some of which you touch)? |
| name: "{{ openshift.common.service_type }}-master-api" | ||
| state: restarted | ||
| when: | ||
| - openshift.master.ha is defined and openshift.master.ha | bool |
There was a problem hiding this comment.
I ❤️ the changes to ANDing in list format
| - restart master | ||
| - restart master api | ||
| - restart master controllers | ||
| - restart master |
There was a problem hiding this comment.
👍 for consistency in indentation
|
|
||
| - command: systemctl daemon-reload | ||
| when: create_ha_unit_files | changed | ||
| when: l_create_ha_unit_files | changed |
There was a problem hiding this comment.
👍 for fixing the variable names to match convention!
openshift-bot
added
the
needs-rebase
ingvagabund
force-pushed
the
polish-openshift-master-role
branch
from
1e870f3 to
8144f53
Compare
|
aos-ci-test |
openshift-bot
removed
the
needs-rebase
ingvagabund
force-pushed
the
polish-openshift-master-role
branch
from
8144f53 to
13c0075
Compare
|
aos-ci-test |
|
@smarterclayton so you know. @tbielawa can you re-review the PR one more time? |
|
[test] |
|
Evaluated for openshift ansible test up to 13c0075 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_openshift_ansible/457/) (Base Commit: b2b9e5e) (PR Branch Commit: 13c0075) |
|
[merge] |
|
Evaluated for openshift ansible merge up to 13c0075 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_openshift_ansible/868/) (Base Commit: a61da58) (PR Branch Commit: 13c0075) |
Let's make the code more readable and closer to "best practices" of writing the Ansible.