linux-system-roles / template

example template for creating new subsystem roles
https://linux-system-roles.github.io/template/
MIT License
13 stars 20 forks source link

To quote stringts or not to quote #81

Closed Tronde closed 2 years ago

Tronde commented 2 years ago

Looking at files like defaults/main.yml and vars/main.yml, I see that strings are being quoted there. When working on another role @richm suggested to me that strings don't need to be quoted. Now I'm a little bit confused.

Looking at the template I would assume that's fine to quote strings. If that's not the case, the quotes should be removed from the var files in the template (I could volunteer to do that). But which way is correct? Please tell me if we have to quote or not to quote strings.

richm commented 2 years ago

Looking at files like defaults/main.yml and vars/main.yml, I see that strings are being quoted there. When working on another role @richm suggested to me that strings don't need to be quoted. Now I'm a little bit confused.

Looking at the template I would assume that's fine to quote strings. If that's not the case, the quotes should be removed from the var files in the template (I could volunteer to do that). But which way is correct? Please tell me if we have to quote or not to quote strings.

The Ansible style recommendations say that strings should not be quoted unless necessary. https://github.com/redhat-cop/automation-good-practices/blob/main/coding_style/README.adoc

Do not use quotes unless you have to, especially for short module-keyword-like strings like present, absent, etc. But do use quotes for user-side strings such as descriptions, names, and messages.

I prefer to go further and say - do not quote anything unless it is necessary. That is, if it is a valid string and is parsable with YAML and yamllint doesn't flag it, then do not quote it (taking into consideration certain Ansible constructs such as someparam: "{{ some_variable }}" that must always be quoted)

So you might say - well, why doesn't the template take these and other recommendations into consideration? One answer is that the template code was in most cases written before these recommendations became codified, and we just haven't kept up with these and other recommendations.

I'm sure a dedicated researcher would find many places in the template code that deviate from https://github.com/redhat-cop/automation-good-practices

nhosoi commented 2 years ago

I've approved https://github.com/linux-system-roles/template/pull/83.

BTW, there are plenty of quoted strings in the existing roles, e.g., https://github.com/linux-system-roles/logging/blob/master/roles/rsyslog/vars/main.yml ... Do we want to clean them up (some day)? Or would the good practices be applied to the new codes only?

richm commented 2 years ago

I've approved #83.

BTW, there are plenty of quoted strings in the existing roles, e.g., https://github.com/linux-system-roles/logging/blob/master/roles/rsyslog/vars/main.yml ... Do we want to clean them up (some day)? Or would the good practices be applied to the new codes only?

I would say that it isn't necessary to remove quoted strings just for the sake of it. However, if you are working on some code for some other reason that uses quoted strings unnecessarily, it is probably good to clean that up as part of the unrelated fix.

Note that system roles code in general probably violates https://github.com/redhat-cop/automation-good-practices in several places - this is just one small part of that.

What I would really like to see is tooling around these practices e.g. you could run yamllint or ansible-lint and it would complain about unnecessarily quoted strings. It's too difficult to enforce all of these manually.

nhosoi commented 2 years ago

What I would really like to see is tooling around these practices e.g. you could run yamllint or ansible-lint and it would complain about unnecessarily quoted strings. It's too difficult to enforce all of these manually.

+100 - it'd be really nice if yamllint/ansible-lint gets smarter and smarter to tell us what to do.