thegeeklab / ansible-doctor

Annotation based documentation for your Ansible roles
https://ansible-doctor.geekdocs.de
GNU General Public License v3.0
123 stars 18 forks source link

vars / defaults - possible naming conflict in code #757

Open hhenkel opened 1 month ago

hhenkel commented 1 month ago

Hi,

thank you for your software! I was toying around with your tool today and was also looking into other tools that are trying to achieve similar things. One thing I'm currently missing is the possibility to add files under vars/ to the readme.

I had a quick glance into the source code to get an understanding how things work and how time consuming it would be to add the functionality.

So the first question would be, if you're open for adding the files under vars/ to the readme? If so I see a naming conflict because you decided to store the data from defaults/ in the dict and everywhere else with the name var - see for example.

I could try to do a PR for renaming all the stuff related to defaults/ if you are considering changing the internal variable names.

xoxys commented 4 weeks ago

Hi, thanks for your feedback.

So the first question would be, if you're open for adding the files under vars/ to the readme?

Sure would be a nice enhancement to support annotations in vars files as well.

If you are considering changing the internal variable

I'm not sure if that's really required. There should be no duplicate names for Ansible variables in vars and defaults anyway. So we should just add parsing for files in the vars directory and store them in the var key of the internal data dict as well. Or am I missing something?

hhenkel commented 4 weeks ago

Sure would be a nice enhancement to support annotations in vars files as well.

Okay - I will look into that then - might take a while as I will be on vacation.

I'm not sure if that's really required. There should be no duplicate names for Ansible variables in vars and defaults anyway. So we should just add parsing for files in the vars directory and store them in the var key of the internal data dict as well. Or am I missing something?

My thought was, that it might be good to separate them, so it is possible to list them independently from the defaults. In our case, when importing/including a role, the variables that are defined under defaults/ are usually over-writable by providing variables to the include_task. Variables under vars/ are normally platform / os specific in our case, but we might have files defining different values for different versions of an os.

I thought about using the key "defaults" and "vars" to make it more obvious in the code. Another topic could be to store the actual filename that contain the variables, maybe like this:

self._data["defaults"]["main.yml"]
self._data["vars"]["redhat-redhat-8.9.yaml"]

Here is an example for my specific use case:

defaults/main.yaml => The "sane" defaults for a role that are independent from the OS. This also includes a variable <role_name>_cis_hardened: false so that the role uses the system defaults. If the target system needs CIS hardening, this is set to true.

As a default we follow the following schema to include var files if they exist via ansible.builtin.first_found:

vars/redhat-redhat-8.9.yaml => OS_Family-Distribution-Distribution_Version
vars/redhat-redhat-8.yaml => OS_Family-Distribution-Distribution_Major_Version
vars/redhat-redhat.yaml => OS_Family-Distribution
vars/redhat.yaml => OS_Family

If CIS hardening is used we read additional var files to overwrite the os default config following this schema:

vars/cis-redhat-redhat-8.9.yaml
vars/cis-redhat-redhat-8.yaml
vars/cis-redhat-redhat.yaml
vars/cis-redhat.yaml

It might be nice to see what different files exist under vars/ and what variables are defined in them.

ansible-mdgen, which creates a lot of pages, even adds information to the documentation where the variable is referenced.

xoxys commented 4 weeks ago

Okay - I will look into that then - might take a while as I will be on vacation.

No worries, enjoy your vacation 🏖️

My thought was, that it might be good to separate them, so it is possible to list them independently from the defaults.

Thanks for the detailed information. Makes sense to me, and I'm also fine to separate vars and defaults in the data dict.