refacto: add fqdn, argspecs and clean task names#380
refacto: add fqdn, argspecs and clean task names#380lutin-malin wants to merge 2 commits intoNETWAYS:mainfrom
Conversation
Donien
left a comment
Donien
left a comment
There was a problem hiding this comment.
I generally like what you did here. Thanks for your efforts!
I believe there is a discussion to be had regarding the style for the collection in the future, @mkayontour.
- FQCN: obvious, we should do this.
- Task names that show where we are in the role's hierarchy: Maybe? Ansible's output should already show the task files, though.
- Prefix internal variables: yes, definitely. Just how exactly...
|
|
||
| icinga2_config_directories: | ||
| - zones.d/main/commands | ||
| - zones.d/main/hosts | ||
| - zones.d/main/services |
There was a problem hiding this comment.
I would not use a default here. The primary zone could be called anything really.
We should rather explain this one better in the documentation (and/or argument spec).
| icinga2_config_directories: | |
| - zones.d/main/commands | |
| - zones.d/main/hosts | |
| - zones.d/main/services |
There was a problem hiding this comment.
Yes indeed, this should not be a default.
| default: ["icinga2"] | ||
| icinga2_packages_dependencies: | ||
| description: | ||
| - list of packages dependancies to be installed in addition to packages |
There was a problem hiding this comment.
- A typo (will ignore further ones in this review. We'll have another round).
- Options can be referenced by other options inside argument spec. Have a look here
| - list of packages dependancies to be installed in addition to packages | |
| - List of package dependencies to be installed in addition to O(icinga2_packages). |
| - OS Specific | ||
| type: list | ||
| elements: str | ||
| ansible_selinux: # naming is wrong, should be prefixed with the role name, e.g. icinga2_selinux |
There was a problem hiding this comment.
You are right regarding this variable name
| --port "{{ __icinga2_ca_host_port | default('5665') }}" | ||
| --trustedcert "{{ icinga2_cert_path }}/trusted-master.crt" | ||
| when: icinga2_ca_host != 'none' | ||
| register: _trusted_master_cert |
There was a problem hiding this comment.
You probably just missed this.
_trusted_master_cert
We should decide whether we use single or double underscores for internal variables, @mkayontour.
But we should keep it consistent.
As far as I know, there is no functional difference between single and double underscores.
I've heard people say "Double underscores lead to the variable being private" which I could not confirm in Ansible (no documentation on this; In Python there is a difference, though).
There was a problem hiding this comment.
If there's no functional difference, which also my current knowledge, I would not change it. @lutin-malin do you have further information on the topic to help make a decision?
| - name: features | api | Ensure icinga2 certificate directory | ||
| ansible.builtin.file: | ||
| path: "{{ icinga2_cert_path }}" | ||
| state: directory | ||
| owner: "{{ icinga2_user }}" | ||
| group: "{{ icinga2_group }}" | ||
| mode: "0750" | ||
|
|
||
| - name: Copy self generated certificates to icinga2 certificate directory | ||
| copy: | ||
| - name: features | api | Copy self generated certificates to icinga2 certificate directory | ||
| ansible.builtin.copy: | ||
| remote_src: no | ||
| src: "{{ _crt.src }}" | ||
| dest: "{{ _crt.dest }}" | ||
| owner: "{{ icinga2_user }}" | ||
| group: "{{ icinga2_group }}" | ||
| notify: check-and-reload-icinga2-service | ||
| loop: "{{ _tmp_crt }}" | ||
| loop: | ||
| - src: "{{ __icinga2_ssl_cacert }}" | ||
| dest: "{{ icinga2_cert_path }}/ca.crt" | ||
| - src: "{{ __icinga2_ssl_key }}" | ||
| dest: "{{ icinga2_cert_path }}/{{ __icinga2_cert_name }}.key" | ||
| - src: "{{ __icinga2_ssl_cert }}" | ||
| dest: "{{ icinga2_cert_path }}/{{ __icinga2_cert_name }}.crt" |
There was a problem hiding this comment.
I love this part where the src/dest pairs are moved to the loop directly! :)
There was a problem hiding this comment.
I need to test this again, I had issues with variables resolving when using variables in keywords like vars/loop, if you are using a omit() filter the variables are not omitted. This is also a few Ansible versions prior and therefore decided to use set_facts often.
But awesome stuff nevertheless.
| - name: feature notification NotificationComponent object | ||
| icinga2_object: | ||
| - name: features | notification | NotificationComponent object | ||
| icinga.icinga.icinga2_object: |
There was a problem hiding this comment.
| icinga.icinga.icinga2_object: | |
| netways.icinga.icinga2_object: |
| - name: feature influxdb OpenTsdbWriter object | ||
| icinga2_object: | ||
| - name: features | opentsdb | OpenTsdbWriter object | ||
| icinga.icinga.icinga2_object: |
There was a problem hiding this comment.
| icinga.icinga.icinga2_object: | |
| netways.icinga.icinga2_object: |
| - name: feature perfdata PerfdataWriter object | ||
| icinga2_object: | ||
| - name: features | perfdata | PerfdataWriter object | ||
| icinga.icinga.icinga2_object: |
There was a problem hiding this comment.
| icinga.icinga.icinga2_object: | |
| netways.icinga.icinga2_object: |
| - name: feature syslog SyslogLogger object | ||
| icinga2_object: | ||
| - name: features | syslog | SyslogLogger object | ||
| icinga.icinga.icinga2_object: |
There was a problem hiding this comment.
| icinga.icinga.icinga2_object: | |
| netways.icinga.icinga2_object: |
| - icinga2_object: | ||
| - name: objects | Convert collected objects to API Object | ||
| when: __icinga2_tmp_objects is defined | ||
| icinga.icinga.icinga2_object: |
There was a problem hiding this comment.
| icinga.icinga.icinga2_object: | |
| netways.icinga.icinga2_object: |
|
Hi @Donien, i've not made any change yet assuming you'd be defining coding styles internaly. I'm not sure if i should adress your comments or wait... ? |
|
Hi @lutin-malin, we've talked about this briefly (code style) but kinda lost track again. For now I don't want your PR to go stale and lose compatibility (again). I've made a few suggestions in the PR. Those ones can just be applied. For everything else:
Afterwards I will have another full look and merge as soon as I can manage. As for the other rules, apart from using the FQCN everywhere I would not touch them (yet). Other argument specs will be part of #321. Thanks again for your contribution (and patience 👀) |
3 participants
replaces the current #364 where i have issue to get the signed CLA recognized.
Hi icinga team,
I've been trying to troubleshoot a deployment issue (only on my side) and ended up looking at your collection.
I found some improvments to be performed, so i took the freedom to give it a try.
Here you find my proposal.
In a nutshell, i have :
moved all modules call to fqdn
prefixed internal variables with __icinga2 to reduce its exposure to extern variable or any unwanted superseed
implemented the argument_specs for role variable validation
done some linting
rearranged the defaults/vars where applicable.
I carefully bear attention to not touch the business logic being executed.
Also, this would be the 1st stone to build automated documentation for antsibull and ansible-doc. (cf #321 )
So far, i've done it only on the icinga2 role.
If this suits you, i could do further on the remaining roles.
Let me know your thoughts.