Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow/enforce underscore prefixed internal variables #3452

Closed
gardar opened this issue May 16, 2023 · 8 comments
Closed

Allow/enforce underscore prefixed internal variables #3452

gardar opened this issue May 16, 2023 · 8 comments
Labels

Comments

@gardar
Copy link

gardar commented May 16, 2023

Summary

Followup to: #3422 (comment)

Internal variables are often prefixed with underscores to distinguish them as in python: https://peps.python.org/pep-0008/#descriptive-naming-styles

The no-role-prefix rule should allow internal variables to have a leading underscore in addition to the role name prefix.
Also perhaps there's no need for the role name prefix on internal variables if all of the internal variables are distinguished with a underscore prefix.

In the prometheus ansible collection roles we prefix the variables in vars/ with a single underscore and then variables which are registered in tasks are prefixed with double underscores.
I've seen the same style used in roles from @geerlingguy and others

@gardar gardar added bug new Triage required labels May 16, 2023
@cidrblock
Copy link
Contributor

I like this idea, always have.

We need to make sure AH will allow it, we can ask @thedoubl3j or @alisonlhart

A couple related things come to mind:

redhat-cop/automation-good-practices#80
ansible-community/community-topics#154

Regardless of whether or not ansible-core ever recognizes the convetion, I think it is sane.

We should (you, sorin, or I) should raise an issue in the COP repo proposing the standard. We work closely with them to make sure we never have conflicting standards and try to stay in sync.

@gardar
Copy link
Author

gardar commented May 16, 2023

While we are on the subject, I wish we could also prefix role names with underscores, to create roles in collections that are like a library, and sourced by other roles for common tasks and such and not supposed to be used directly.

@cidrblock cidrblock removed the new Triage required label May 17, 2023
@richm
Copy link

richm commented May 17, 2023

The no-role-prefix rule should allow internal variables to have a leading underscore in addition to the role name prefix.

I would prefer to allow 1 or 2 leading underscores - in system roles we use two e.g. https://github.com/linux-system-roles/logging/blob/main/roles/rsyslog/tasks/deploy_nolog.yml#L39
because of the recommended practices - https://github.com/redhat-cop/automation-good-practices/tree/main/roles

Moreover, internal variables (those that are not expected to be set by users) are to be prefixed
by two underscores: __foo_variable.

where foo is the rolename

@ssbarnea ssbarnea added the feedback-needed Divergent oppinions, additional feedback is desired in order to unblock it. label May 18, 2023
@erikgb
Copy link
Contributor

erikgb commented May 19, 2023

Please fix this ASAP. The latest release (6.16.1) broke the following code:

- name: Assert Secret
  ansible.builtin.assert:
    that:
      - test_tls_secret_secret.type == 'kubernetes.io/tls'
      - "'tls.crt' in test_tls_secret_secret.data"
      - "'tls.key' in test_tls_secret_secret.data"
  vars:
    test_tls_secret_secret: "{{ test_tls_secret_secret_list.resources[0] }}"

How am I supposed to fix the following new ansible-lint errors?

var-naming[no-role-prefix]: Variables names from within roles should use role_name_ as a prefix. (vars: __file__)
molecule/tls_secret/roles/test_tls_secret/tasks/main.yml:19 Task/Handler: Assert Secret

var-naming[no-role-prefix]: Variables names from within roles should use role_name_ as a prefix. (vars: __line__)
molecule/tls_secret/roles/test_tls_secret/tasks/main.yml:19 Task/Handler: Assert Secret

What kind of versioning is ansible-lint practicing? It is definitely not SemVer.....

@drybjed
Copy link
Contributor

drybjed commented May 19, 2023

I can confirm the above case, here's a test role which triggers the warning.

@jeremyspencer39171
Copy link

I encountered the same thing just now, thought I'd lost my marbles.

@drybjed
Copy link
Contributor

drybjed commented May 21, 2023

The issue should be fixed in the latest ansible-lint release (v6.16.2), it works correctly now with my playbooks and roles.

@ssbarnea ssbarnea removed the feedback-needed Divergent oppinions, additional feedback is desired in order to unblock it. label May 22, 2023
@alexdyukov
Copy link

alexdyukov commented May 22, 2023

@drybjed @ssbarnea There is a issue with register keyword also. Code like this:

---
- name: Add repository
  ansible.builtin.yum_repository:
    name: testrole
    description: Testrole
    baseurl: "{{ testrole_url }}"
    gpgcheck: false
    enabled: true
    state: present
  register: _repo

throws the error:

var-naming[no-role-prefix]: Variables names from within roles should use role_name_ as a prefix. (register: _repo)

As i can see the topic, it mentions the error as part of issue:
Also perhaps there's no need for the role name prefix on internal variables if all of the internal variables are distinguished with a underscore prefix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants