bbaassssiiee commented 5 years ago

152 reported the output of ansible-lint of a playbook with all these roles. There was clearly room for improvement, and I did some power-editing to refactor all the simple code-style issues. Each commit in this PR addresses one type of issue reported by ansible-lint, on all the roles.

After merging this, there is still work to do by the regular maintainers to bring this repo up to the ansible-galaxy standards. A few long lines could still be simplified. And then please address the remaining issues in several places:

[x] [201] Trailing whitespace.
[ ] [204] Lines should be no longer than 160 chars.
[x] [206] Variables should have spaces before and after: {{ var }}.
[x] [305] Use shell only when shell functionality is required.
[x] [502] All tasks should be named.
[x] [701] Role info should contain platforms.
[x] [702] Galaxy tags must contain lowercase letters and digits.
[x] [703] Should change default metadata.
[x] [301] Commands should not change things if nothing needs doing.
[ ] [303] curl used in place of get_url or uri module.
[x] [306] Shells that use pipes should set the pipefail option.
[ ] [503] Tasks that run when changed should likely be handlers.
[x] [601] Don't compare to literal True/False.

bbaassssiiee commented 5 years ago

Remaining issues:

[503] Tasks that run when changed should likely be handlers
isam-ansible-roles/add_system_alerts_rsyslog/tasks/main.yml:20
Task/Handler: Enable System Alert - Created in previous Step

[503] Tasks that run when changed should likely be handlers
isam-ansible-roles/add_system_alerts_smtp/tasks/main.yml:22
Task/Handler: Enable System Alert - Created in previous Step

[503] Tasks that run when changed should likely be handlers
isam-ansible-roles/add_system_alerts_snmp/tasks/main.yml:32
Task/Handler: Enable System Alert - Created in previous Step

[301] Commands should not change things if nothing needs doing
isam-ansible-roles/bootstrap_local/tasks/bootstrap.yml:40
Task/Handler: waiting for VM to install isam image and shutdown

[303] curl used in place of get_url or uri module
isam-ansible-roles/bootstrap_local/tasks/bootstrap.yml:97
Task/Handler: wait for LMI up

[204] Lines should be no longer than 160 chars
isam-ansible-roles/execute_compare/tasks/main.yml:34
  when: (execute_compare_fed and ('federation' in master_web_runtime['ansible_facts']['activations'])) or (execute_compare_mga and ('mga' in master_web_runtime['ansible_facts']['activations']))

[204] Lines should be no longer than 160 chars
isam-ansible-roles/gen_report/tasks/main.yml:4
    dest: "{{ gen_report_dir }}/{{ gen_report_template.partition('.')[0] }}_{{ ansible_date_time.date }}-{{ ansible_date_time.hour }}-{{ ansible_date_time.minute }}-{{ ansible_date_time.second }}.html"

[503] Tasks that run when changed should likely be handlers
isam-ansible-roles/sanity_checks/tasks/main.yml:13
Task/Handler: If Appliance has pending changes - then fail

ram-ibm commented 5 years ago

The volume of changes means this is going to take some time to merge. Please be patient.

bbaassssiiee commented 5 years ago

Tip: This PR is easier to review if you look at the individual commits instead of at the total diff.

IBM-Security / isam-ansible-roles

🚨 ansible-lint code quality improvements. #156

152 reported the output of ansible-lint of a playbook with all these roles. There was clearly room for improvement, and I did some power-editing to refactor all the simple code-style issues. Each commit in this PR addresses one type of issue reported by ansible-lint, on all the roles.