From ccf9fed2f39d210f7006edc07b63ecf09d4c2841 Mon Sep 17 00:00:00 2001 From: Kedar Bidarkar Date: Thu, 19 Jul 2018 18:28:23 +0530 Subject: [PATCH] Updated as per comments. *) Fixed as per yamllint *) Fixed as per ansible-review and ansible-lint *) Moved inventory file to inventory_files *) Added standards.py under rules/ansible-review. *) Updated ansible.cfg as per suggestions *) Added some best practices to CONTRIBUTING.md Signed-off-by: Kedar Bidarkar --- .gitignore | 4 +- CONTRIBUTING.md | 88 ++++- README.md | 9 +- ansible.cfg | 4 +- .../inventory.sample | 0 requirements.yml | 1 + roles/partition-disk/defaults/main.yml | 2 +- roles/partition-disk/handlers/main.yml | 2 +- roles/partition-disk/meta/main.yml | 46 +-- .../tasks/extend_root_partition.yaml | 16 - .../tasks/extend_root_partition.yml | 16 + roles/partition-disk/tasks/main.yml | 4 +- .../tasks/remove_home_partition.yaml | 18 - .../tasks/remove_home_partition.yml | 19 ++ roles/partition-disk/vars/main.yml | 2 +- rules/ansible-review/standards.py | 321 ++++++++++++++++++ 16 files changed, 467 insertions(+), 85 deletions(-) rename inventory.sample => inventory_files/inventory.sample (100%) delete mode 100644 roles/partition-disk/tasks/extend_root_partition.yaml create mode 100644 roles/partition-disk/tasks/extend_root_partition.yml delete mode 100644 roles/partition-disk/tasks/remove_home_partition.yaml create mode 100644 roles/partition-disk/tasks/remove_home_partition.yml create mode 100644 rules/ansible-review/standards.py diff --git a/.gitignore b/.gitignore index 44df35e..9f30408 100644 --- a/.gitignore +++ b/.gitignore @@ -6,6 +6,7 @@ log/ logs-*/ # Byte-compiled / optimized / DLL files +rules/ansible-review/__pycache__ __pycache__/ venv/ env/ @@ -52,10 +53,11 @@ coverage.xml .pydevproject .settings/ - # Sphinx documentation docs/_build/ # Ansible *.retry +# Galaxy Roles +galaxy_roles/ diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fa06a4f..85dc977 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -16,12 +16,94 @@ git push origin your-branch-name You will then be able to create a pull request from your commit. All fixes or new addition to the project should be accompanied by tests that -should PASS. +should PASS. Run the role/playbook twice, to make sure it's idempotent. Feel free to raise issues in the repo if you feel a fix or new feature support is needed. Standards ========= -Install and Run [ansible-lint](https://github.com/willthames/ansible-lint) against your code changes. -Also, Install and Run [ansible-review](https://github.com/willthames/ansible-review) against your code changes. +Install and Run [ansible-lint](https://github.com/willthames/ansible-lint) against your code. + +``` +ansible-lint playbooks/install/satellite_63.yml +``` + +Install and Run [ansible-review](https://github.com/willthames/ansible-review) against your code. + +``` +git ls-files | grep -i yml | xargs ansible-review -d rules/ansible-review/ +``` + +Probably you may also want to run yamllint. + +``` +git ls-files | grep -i yml | xargs yamllint +``` + +Some Best Practices +=================== + +Some Best Practices to follow while writing roles/tasks, + +1. Ansible Roles: + + a. Avoid one huge role. + + b. One Role, One Goal. + + c. Avoid various tasks within a role, that are not related. + + d. Use `ansible-galaxy init role_name` to begin with roles. + + e. Following the ansible-galaxy pattern for roles would later help make + the roles push to ansible-galaxy or make use of the role elsewhere easily + + f. Include/Update the requirements.yml file for any external roles used in + the project. + + g. Make sure there is a Readme.md file for each role created. + + h. Test/Run the role at-least twice, to make sure it's idempotent. + (i.e, the role/playbook should not fail upon re-running it for the same + host.) + +2. Ansible Vars: + + a. Set a default value for every variable, they are like examples. + + b. Prefer scalar variables over dictionary type variables as much as + possible. + +3. Use Multi Line YAML notation, instead of key=value + + Vertical code is easier to read and maintain. + +4. Use ansible-vault for secret passwords + + Example: Ec2, subscriptions password + +5. Always seek out an ansible module first, instead of running a command as + part of roles/tasks. + + a. If you ever need to use "command", try to make the command module + idempotent. In order to achieve idempotence, you could use the attributes + "creates" and "removes". + + b. Avoid shell module as far as possible and should be used only for I/O + redirect if in case required. + + c. Idempotence: An operation is idempotent if the result of performing it + once is exactly the same as the result of performing it repeatedly + without any intervening actions. + +6. Have consistency in Naming Convention: + + a. Tasks, playbooks, Variables, Roles and Tags. + + b. Directory Structure/layout. + +7. Use handlers while restarting services. + +8. Make use of the ansible_variables (i.e facts fetched from host by setup module ) as + much as possible. diff --git a/README.md b/README.md index 9a0777d..64af06c 100644 --- a/README.md +++ b/README.md @@ -8,9 +8,9 @@ Ansible playbooks for Satellite 6 systems' management. This repository will be Python3.6 based. -You need Ansible installed. +You need Ansible 2.5.0+ installed. -We also recommend to install ansible-lint and ansible-review. +We also recommend to install yamllint, ansible-lint and ansible-review. ### Configuration @@ -48,4 +48,7 @@ $ ansible-playbook -i inventory playbooks/install/satellite_63.yml ## Contributing Please read [CONTRIBUTING.md](https://github.com/SatelliteQE/ansible-satellite6/blob/master/CONTRIBUTING.md) if you wish to contribute. - + +## License + +ansible-satellite6 is licensed under [GPL version 3](https://github.com/SatelliteQE/ansible-satellite6/blob/master/LICENSE). diff --git a/ansible.cfg b/ansible.cfg index eea9a56..dc5cfeb 100644 --- a/ansible.cfg +++ b/ansible.cfg @@ -2,5 +2,7 @@ library = foreman-ansible-modules/modules module_utils = foreman-ansible-modules/module_utils roles_path = $PWD/galaxy_roles:$PWD/roles -inventory = inventory +inventory = inventory_files/inventory retry_files_enabled = False +stdout_callback = yaml +display_skipped_hosts = False diff --git a/inventory.sample b/inventory_files/inventory.sample similarity index 100% rename from inventory.sample rename to inventory_files/inventory.sample diff --git a/requirements.yml b/requirements.yml index 6d95021..1ed13ce 100644 --- a/requirements.yml +++ b/requirements.yml @@ -1,2 +1,3 @@ +--- # from galaxy # - src: username.rolename diff --git a/roles/partition-disk/defaults/main.yml b/roles/partition-disk/defaults/main.yml index e7e4263..f4287d9 100644 --- a/roles/partition-disk/defaults/main.yml +++ b/roles/partition-disk/defaults/main.yml @@ -1,2 +1,2 @@ --- -# defaults file for partition-disk \ No newline at end of file +# defaults file for partition-disk diff --git a/roles/partition-disk/handlers/main.yml b/roles/partition-disk/handlers/main.yml index 978d0df..9a9d418 100644 --- a/roles/partition-disk/handlers/main.yml +++ b/roles/partition-disk/handlers/main.yml @@ -1,2 +1,2 @@ --- -# handlers file for partition-disk \ No newline at end of file +# handlers file for partition-disk diff --git a/roles/partition-disk/meta/main.yml b/roles/partition-disk/meta/main.yml index eeaf238..a5c90ea 100644 --- a/roles/partition-disk/meta/main.yml +++ b/roles/partition-disk/meta/main.yml @@ -1,50 +1,20 @@ +--- +# Standards: 0.2 galaxy_info: author: Satellite QE Team description: Satellite QE Team - company: RedHat + company: Red Hat - # If the issue tracker for your role is not on github, uncomment the - # next line and provide a value - # issue_tracker_url: http://example.com/issue/tracker - - # Some suggested licenses: - # - BSD (default) - # - MIT - # - GPLv2 - # - GPLv3 - # - Apache - # - CC-BY license: GPLv3 - min_ansible_version: 1.2 - - # If this a Container Enabled role, provide the minimum Ansible Container version. - # min_ansible_container_version: + min_ansible_version: 2.5.0 - # Optionally specify the branch Galaxy will use when accessing the GitHub - # repo for this role. During role install, if no tags are available, - # Galaxy will use this branch. During import Galaxy will access files on - # this branch. If Travis integration is configured, only notifications for this - # branch will be accepted. Otherwise, in all cases, the repo's default branch - # (usually master) will be used. - #github_branch: - - # - # platforms is a list of platforms, and each platform has a name and a list of versions. - # platforms: - - name: RHEL - versions: - - 7 + - name: RHEL + versions: + - 7 galaxy_tags: [] - # List tags for your role here, one per line. A tag is a keyword that describes - # and categorizes the role. Users find roles by searching for tags. Be sure to - # remove the '[]' above, if you add tags to this list. - # - # NOTE: A tag is limited to a single word comprised of alphanumeric characters. - # Maximum 20 tags per role. + dependencies: [] - # List your role dependencies here, one per line. Be sure to remove the '[]' above, - # if you add dependencies to this list. diff --git a/roles/partition-disk/tasks/extend_root_partition.yaml b/roles/partition-disk/tasks/extend_root_partition.yaml deleted file mode 100644 index c23bd3e..0000000 --- a/roles/partition-disk/tasks/extend_root_partition.yaml +++ /dev/null @@ -1,16 +0,0 @@ - - name: "Extend root partition LV to all available VG space" - when: item.key in "lv_root" - lvol: - vg: "{{ item.value.vg }}" - lv: "{{ item.key }}" - size: +100%FREE - with_dict: "{{ ansible_lvm.lvs }}" - - - name: "Resize the root filesystem" - when: item.key in "lv_root" - filesystem: - dev: "/dev/{{ item.value.vg }}/{{ item.key }}" - fstype: xfs - resizefs: yes - with_dict: "{{ ansible_lvm.lvs }}" -... diff --git a/roles/partition-disk/tasks/extend_root_partition.yml b/roles/partition-disk/tasks/extend_root_partition.yml new file mode 100644 index 0000000..05f5afb --- /dev/null +++ b/roles/partition-disk/tasks/extend_root_partition.yml @@ -0,0 +1,16 @@ +--- +- name: "Extend root partition LV to all available VG space" + when: item.key in "lv_root" + lvol: + vg: "{{ item.value.vg }}" + lv: "{{ item.key }}" + size: +100%FREE + with_dict: "{{ ansible_lvm.lvs }}" + +- name: "Resize the root filesystem" + when: item.key in "lv_root" + filesystem: + dev: "/dev/{{ item.value.vg }}/{{ item.key }}" + fstype: xfs + resizefs: true + with_dict: "{{ ansible_lvm.lvs }}" diff --git a/roles/partition-disk/tasks/main.yml b/roles/partition-disk/tasks/main.yml index 540ad8e..cc8cffd 100644 --- a/roles/partition-disk/tasks/main.yml +++ b/roles/partition-disk/tasks/main.yml @@ -1,4 +1,4 @@ --- # tasks file for partition-disk -- include_tasks: remove_home_partition.yaml -- include_tasks: extend_root_partition.yaml +- include_tasks: remove_home_partition.yml +- include_tasks: extend_root_partition.yml diff --git a/roles/partition-disk/tasks/remove_home_partition.yaml b/roles/partition-disk/tasks/remove_home_partition.yaml deleted file mode 100644 index f71d680..0000000 --- a/roles/partition-disk/tasks/remove_home_partition.yaml +++ /dev/null @@ -1,18 +0,0 @@ ---- - - name: "Unmount /home partition" - mount: - name: /home - state: unmounted - - name: "Remove /home entry from /etc/fstab" - mount: - name: /home - state: absent - - name: "Remove /home Logical Volume" - when: item.key in "lv_home" - lvol: - vg: "{{ item.value.vg }}" - lv: "{{ item.key }}" - state: absent - force: yes - with_dict: "{{ ansible_lvm.lvs }}" -... diff --git a/roles/partition-disk/tasks/remove_home_partition.yml b/roles/partition-disk/tasks/remove_home_partition.yml new file mode 100644 index 0000000..04a12ee --- /dev/null +++ b/roles/partition-disk/tasks/remove_home_partition.yml @@ -0,0 +1,19 @@ +--- +- name: "Unmount /home partition" + mount: + name: /home + state: unmounted + +- name: "Remove /home entry from /etc/fstab" + mount: + name: /home + state: absent + +- name: "Remove /home Logical Volume" + when: item.key in "lv_home" + lvol: + vg: "{{ item.value.vg }}" + lv: "{{ item.key }}" + state: absent + force: true + with_dict: "{{ ansible_lvm.lvs }}" diff --git a/roles/partition-disk/vars/main.yml b/roles/partition-disk/vars/main.yml index 99f5a64..52ea25e 100644 --- a/roles/partition-disk/vars/main.yml +++ b/roles/partition-disk/vars/main.yml @@ -1,2 +1,2 @@ --- -# vars file for partition-disk \ No newline at end of file +# vars file for partition-disk diff --git a/rules/ansible-review/standards.py b/rules/ansible-review/standards.py new file mode 100644 index 0000000..4bb2272 --- /dev/null +++ b/rules/ansible-review/standards.py @@ -0,0 +1,321 @@ +import codecs +import os +import yaml + +from ansiblereview import Result, Error, Standard, lintcheck +from ansiblereview.utils.yamlindent import yamlreview +from ansiblereview.inventory import parse, no_vars_in_host_file +from ansiblereview.code import code_passes_flake8 +from ansiblereview.vars import repeated_vars +from ansiblereview.playbook import repeated_names +from ansiblereview.rolesfile import yamlrolesfile +from ansiblereview.tasks import yaml_form_rather_than_key_value +from ansiblereview.groupvars import same_variable_defined_in_competing_groups +from ansiblelint.utils import parse_yaml_linenumbers + + +def rolesfile_contains_scm_in_src(candidate, settings): + result = Result(candidate.path) + if candidate.path.endswith(".yml") and os.path.exists(candidate.path): + try: + with codecs.open(candidate.path, mode='rb', encoding='utf-8') as f: + roles = parse_yaml_linenumbers(f.read(), candidate.path) + for role in roles: + if '+' in role.get('src'): + error = Error(role['__line__'], "Use scm key rather " + "than src: scm+url") + result.errors.append(error) + except Exception as e: + result.errors = [Error(None, "Cannot parse YAML from %s: %s" % + (candidate.path, str(e)))] + return result + + +def files_should_have_actual_content(candidate, settings): + errors = [] + with codecs.open(candidate.path, mode='rb', encoding='utf-8') as f: + content = yaml.safe_load(f.read()) + if not content: + errors = [Error(None, "%s appears to have no useful content" % candidate)] + return Result(candidate.path, errors) + + +def host_vars_exist(candidate, settings): + return Result(candidate.path, [Error(None, "Host vars are generally " + "not required")]) + + +def noop(candidate, settings): + return Result(candidate.path) + + +rolesfile_should_be_in_yaml = Standard(dict( + name="Roles file should be in yaml format", + check=yamlrolesfile, + types=["rolesfile"] +)) + +role_must_contain_meta_main = Standard(dict( + name="Roles must contain suitable meta/main.yml", + check=lintcheck('EXTRA0012'), + types=["meta"] +)) + +role_meta_main_must_contain_info = Standard(dict( + name="Roles meta/main.yml must contain important info", + check=lintcheck('EXTRA0013'), + types=["meta"] +)) + +variables_should_contain_whitespace = Standard(dict( + name="Variable uses should contain whitespace", + check=lintcheck('EXTRA0001'), + types=["playbook", "task", "handler", "rolevars", + "hostvars", "groupvars", "template"] +)) + +commands_should_be_idempotent = Standard(dict( + name="Commands should be idempotent", + check=lintcheck('ANSIBLE0012'), + types=["playbook", "task"] +)) + +commands_should_not_be_used_in_place_of_modules = Standard(dict( + name="Commands should not be used in place of modules", + check=lintcheck('ANSIBLE0006,ANSIBLE0007'), + types=["playbook", "task", "handler"] +)) + +package_installs_should_not_use_latest = Standard(dict( + name="Package installs should use present, not latest", + check=lintcheck('ANSIBLE0010'), + types=["playbook", "task", "handler"] +)) + +use_shell_only_when_necessary = Standard(dict( + name="Shell should only be used when essential", + check=lintcheck('ANSIBLE0013'), + types=["playbook", "task", "handler"] +)) + +files_should_be_indented = Standard(dict( + name="YAML should be correctly indented", + check=yamlreview, + types=["playbook", "task", "handler", "rolevars", + "hostvars", "groupvars", "meta"] +)) + +inventory_must_parse = Standard(dict( + name="Inventory must be parseable", + check=parse, + types=["inventory"] +)) + +inventory_hostfiles_should_not_contain_vars = Standard(dict( + name="Inventory host files should not " + "contain variable stanzas ([group:vars])", + check=no_vars_in_host_file, + types=["inventory"] +)) + +code_should_meet_flake8 = Standard(dict( + name="Python code should pass flake8", + check=code_passes_flake8, + types=["code"] +)) + +tasks_are_named = Standard(dict( + name="Tasks and handlers must be named", + check=lintcheck('ANSIBLE0011'), + types=["playbook", "task", "handler"], +)) + +tasks_are_uniquely_named = Standard(dict( + name="Tasks and handlers must be uniquely named within a single file", + check=repeated_names, + types=["playbook", "task", "handler"], +)) + +vars_are_not_repeated_in_same_file = Standard(dict( + name="Vars should only occur once per file", + check=repeated_vars, + types=["rolevars", "hostvars", "groupvars"], +)) + +no_command_line_environment_variables = Standard(dict( + name="Environment variables should be passed through the environment key", + check=lintcheck('ANSIBLE0014'), + types=["playbook", "task", "handler"] +)) + +no_lineinfile = Standard(dict( + name="Lineinfile module should not be used as it suggests " + "more than one thing is managing a file", + check=lintcheck('EXTRA0002'), + types=["playbook", "task", "handler"] +)) + +become_rather_than_sudo = Standard(dict( + name="Use become/become_user/become_method rather than sudo/sudo_user", + check=lintcheck('ANSIBLE0008'), + types=["playbook", "task", "handler"] +)) + +use_yaml_rather_than_key_value = Standard(dict( + name="Use YAML format for tasks and handlers rather than key=value", + check=yaml_form_rather_than_key_value, + types=["playbook", "task", "handler"] +)) + +roles_scm_not_in_src = Standard(dict( + name="Use scm key rather than src: scm+url", + check=rolesfile_contains_scm_in_src, + types=["rolesfile"] +)) + +files_should_not_be_purposeless = Standard(dict( + name="Files should contain useful content", + check=files_should_have_actual_content, + types=["playbook", "task", "handler", "rolevars", "defaults", "meta"] +)) + +playbooks_should_not_contain_logic = Standard(dict( + name="Playbooks should not contain logic (vars, tasks, handlers)", + check=lintcheck('EXTRA0008'), + types=["playbook"] +)) + +host_vars_should_not_be_present = Standard(dict( + name="Host vars should not be present", + check=host_vars_exist, + types=["hostvars"] +)) + +with_items_bare_words = Standard(dict( + name="bare words are deprecated for with_items", + check=lintcheck('ANSIBLE0015'), + types=["task", "handler", "playbook"], + version="0.0" +)) + +file_permissions_are_octal = Standard(dict( + name="octal file permissions should start with a leading zero", + check=lintcheck('ANSIBLE0009'), + types=["task", "handler", "playbook"] +)) + +inventory_hostsfile_has_group_vars = Standard(dict( + name="inventory file should not contain group variables", + check=lintcheck('EXTRA0009'), + types=["inventory"] +)) + +inventory_hostsfile_has_host_vars = Standard(dict( + name="inventory file should not contain host variables " + "(except e.g. ansible_host, ansible_user, etc.)", + check=lintcheck('EXTRA0010'), + types=["inventory"] +)) + +test_matching_groupvar = Standard(dict( + check=same_variable_defined_in_competing_groups, + name="Same variable defined in siblings", + types=["groupvars"] +)) + +hosts_should_not_be_localhost = Standard(dict( + check=lintcheck('EXTRA0007'), + name="Use connection: local rather than hosts: localhost", + types=["playbook"] +)) + +# tasks_should_not_use_action = + +use_handlers_rather_than_when_changed = Standard(dict( + check=lintcheck('ANSIBLE0016'), + name="Use handlers rather than when: changed in tasks", + types=['task', 'playbook'] +)) + +most_files_shouldnt_have_tabs = Standard(dict( + check=lintcheck('EXTRA0005'), + name="Don't use tabs in almost anything that isn't a Makefile", + types=["playbook", "task", "handler", "rolevars", "defaults", "meta", + "code", "groupvars", "hostvars", "inventory", "doc", "template", + "file"] +)) + +dont_delegate_to_localhost = Standard(dict( + check=lintcheck('EXTRA0004'), + name="Use connection: local rather than delegate_to: localhost", + types=["playbook", "task", "handler"] +)) + +become_user_should_have_become = Standard(dict( + check=lintcheck('ANSIBLE0017'), + name="become_user should be accompanied by become", + types=["playbook", "task", "handler"] +)) + +dont_compare_to_literal_bool = Standard(dict( + check=lintcheck('EXTRA0014'), + name="Don't compare to True or False - use `when: var` or `when: not var`", + types=["playbook", "task", "handler", "template"] +)) + +dont_compare_to_empty_string = Standard(dict( + check=lintcheck('EXTRA0015'), + name="Don't compare to \"\" - use `when: var` or `when: not var`", + types=["playbook", "task", "handler", "template"] +)) + +# Update this every time standards version increase +latest_version = Standard(dict( + check=noop, + name="No-op check to ensure latest standards version is set", + version="0.2", + types=[] +)) + +ansible_min_version = '2.1' +ansible_review_min_version = '0.12.0' +ansible_lint_min_version = '3.4.0' + +standards = [ + rolesfile_should_be_in_yaml, + role_must_contain_meta_main, + role_meta_main_must_contain_info, + become_rather_than_sudo, + variables_should_contain_whitespace, + commands_should_be_idempotent, + commands_should_not_be_used_in_place_of_modules, + package_installs_should_not_use_latest, + # files_should_be_indented, + use_shell_only_when_necessary, + inventory_must_parse, + inventory_hostfiles_should_not_contain_vars, + code_should_meet_flake8, + tasks_are_named, + tasks_are_uniquely_named, + vars_are_not_repeated_in_same_file, + no_command_line_environment_variables, + no_lineinfile, + use_yaml_rather_than_key_value, + roles_scm_not_in_src, + files_should_not_be_purposeless, + playbooks_should_not_contain_logic, + host_vars_should_not_be_present, + with_items_bare_words, + file_permissions_are_octal, + inventory_hostsfile_has_host_vars, + inventory_hostsfile_has_group_vars, + test_matching_groupvar, + hosts_should_not_be_localhost, + dont_delegate_to_localhost, + most_files_shouldnt_have_tabs, + use_handlers_rather_than_when_changed, + become_user_should_have_become, + dont_compare_to_empty_string, + dont_compare_to_literal_bool, + latest_version, +]