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

E106: (Role name {} does not match pattern) should not be applied to non-collection roles #966

Closed
nre-ableton opened this issue Aug 18, 2020 · 14 comments · Fixed by #980
Closed
Labels
new Triage required

Comments

@nre-ableton
Copy link
Contributor

nre-ableton commented Aug 18, 2020

Summary

The explanation for violation 106 links to this Ansible documentation page, which details the requirements for collection role names. However, higher up in that same page, a collection role is defined as:

A collection must have a galaxy.yml file that contains the necessary information to build a collection artifact. See Collection Galaxy metadata structure for details.

It seems to me that rule 106 should not be enforced in a galaxy.yml file is not present in the role's root directory. If such a file is absent, then the role is just a regular role and not a collection role, and these naming requirements should not apply.

Issue Type
  • Bug Report
Ansible and Ansible Lint details

ansible 2.9.12
ansible-lint 4.3.0

  • ansible installation method: pip
  • ansible-lint installation method: pip
@nre-ableton nre-ableton added priority/medium new Triage required labels Aug 18, 2020
@nre-ableton nre-ableton changed the title Violation 106 (Role name {} does not match ^[a-z][a-z0-9_]+$ pattern) should not be applied to non-collection roles Violation 106 (Role name {} does not match pattern) should not be applied to non-collection roles Aug 18, 2020
@ssbarnea
Copy link
Member

@nre-ableton You are welcomed to propose a PR for this. I personally did not bother because it was really easy to add exclusion.

I would even go so far to believe that this rule is benefit even for those that do not have yet a collection, as it would help them add new roles that could be packaged at some point, instead of creating more tech-debt.

Keep in mind: that applies to private roles too, not only those published to galaxy server.

@nre-ableton
Copy link
Contributor Author

@ssbarnea Yes, that's correct. I didn't mention this, but I discovered this bug with a project that has several private roles which are not published on Galaxy. I think it would be useful for this case as well.

I'll see if I can come up with a PR to solve it. I'm currently having a bit of trouble figuring out how to run the tests locally etc., but I'll try to sort it out when I have a spare moment.

guits added a commit to ceph/ceph-ansible that referenced this issue Aug 18, 2020
Let's pin to 4.2.0

(because of ansible/ansible-lint/issues/966)

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
guits added a commit to ceph/ceph-ansible that referenced this issue Aug 18, 2020
Let's pin to 4.2.0

(because of ansible/ansible-lint/issues/966)

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
@ssbarnea ssbarnea changed the title Violation 106 (Role name {} does not match pattern) should not be applied to non-collection roles E106: (Role name {} does not match pattern) should not be applied to non-collection roles Aug 18, 2020
guits added a commit to ceph/ceph-ansible that referenced this issue Aug 18, 2020
Let's pin to 4.2.0

(because of ansible/ansible-lint/issues/966)

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
guits added a commit to ceph/ceph-ansible that referenced this issue Aug 18, 2020
Let's pin to 4.2.0

(because of ansible/ansible-lint/issues/966)

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
dsavineau pushed a commit to ceph/ceph-ansible that referenced this issue Aug 18, 2020
Let's pin to 4.2.0

(because of ansible/ansible-lint/issues/966)

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
mergify bot pushed a commit to ceph/ceph-ansible that referenced this issue Aug 18, 2020
Let's pin to 4.2.0

(because of ansible/ansible-lint/issues/966)

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
(cherry picked from commit 04d77dc)
guits added a commit to ceph/ceph-ansible that referenced this issue Aug 18, 2020
Let's pin to 4.2.0

(because of ansible/ansible-lint/issues/966)

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
(cherry picked from commit 04d77dc)
guits added a commit to ceph/ceph-ansible that referenced this issue Aug 18, 2020
Let's pin to 4.2.0

(because of ansible/ansible-lint/issues/966)

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
(cherry picked from commit 04d77dc)
dsavineau pushed a commit to ceph/ceph-ansible that referenced this issue Aug 18, 2020
Let's pin to 4.2.0

(because of ansible/ansible-lint/issues/966)

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
(cherry picked from commit 04d77dc)
@geerlingguy
Copy link

Yeah... now I'm getting this error on every single one of my roles, for example: https://travis-ci.org/github/geerlingguy/ansible-role-solr/jobs/719707367#L489

I'm aware I can add a skip to this rule, but it seems like a very radical departure from this project's goal to, in one minor release, immediately cancel out the ability to use it with the 25,000+ roles that are on Galaxy and have them fail on a rule that's only supposed to apply to collection roles.

@geerlingguy
Copy link

(Aside: roles with dots in there names are perfectly valid for private roles that are not part of a collection... so why are we so strict with this rule? There is one and only one place where the rule would apply, which is a role inside a collection. And that's something exceedingly rare so far (I only see a few dozen roles-in-collections in the wild so far, because it's so much easier to work with roles on the playbook level or standalone on Galaxy).)

@rgarrigue
Copy link

This is nuking dozens of tests here, our roles repositories are named "ansible-role-company-app-name" which becomes "company.app-name" once installed through our requirements.yml. So far it seemed like a (the ?) very standard way to go.

Why is it a non 0 return code instead of a warning ?

This kind of opiniated hard enforcement only lead to disabling tons of rules and frozen dependencies, which lead to tech debt keeping people unaware of the best practise / code quality issues. If a warning is not hard enough in your usage, add a "strict" switch to turn them into error.

hswong3i added a commit to alvistack/ansible-role-ansible that referenced this issue Aug 21, 2020
hswong3i added a commit to alvistack/ansible-role-bamboo that referenced this issue Aug 21, 2020
hswong3i added a commit to alvistack/ansible-role-bitbucket that referenced this issue Aug 21, 2020
hswong3i added a commit to alvistack/ansible-role-blender that referenced this issue Aug 21, 2020
hswong3i added a commit to alvistack/ansible-role-bootstrap that referenced this issue Aug 21, 2020
hswong3i added a commit to alvistack/ansible-role-buildah that referenced this issue Aug 21, 2020
hswong3i added a commit to alvistack/ansible-role-catatonit that referenced this issue Aug 21, 2020
hswong3i added a commit to alvistack/ansible-role-ceph_common that referenced this issue Aug 21, 2020
hswong3i added a commit to alvistack/ansible-role-ceph_mds that referenced this issue Aug 21, 2020
hswong3i added a commit to alvistack/docker-openjdk that referenced this issue Aug 21, 2020
hswong3i added a commit to alvistack/docker-python that referenced this issue Aug 21, 2020
hswong3i added a commit to alvistack/docker-mariadb that referenced this issue Aug 21, 2020
hswong3i added a commit to alvistack/docker-postfix that referenced this issue Aug 21, 2020
hswong3i added a commit to alvistack/docker-jira that referenced this issue Aug 21, 2020
hswong3i added a commit to alvistack/docker-sshd that referenced this issue Aug 21, 2020
hswong3i added a commit to alvistack/docker-opensuse that referenced this issue Aug 21, 2020
hswong3i added a commit to alvistack/docker-postgres that referenced this issue Aug 21, 2020
hswong3i added a commit to alvistack/docker-ubuntu that referenced this issue Aug 21, 2020
hswong3i added a commit to alvistack/docker-node that referenced this issue Aug 21, 2020
hswong3i added a commit to alvistack/docker-php-fpm that referenced this issue Aug 21, 2020
@ssbarnea
Copy link
Member

@rgarrigue Can you please include a reproducible example? Galaxy install path should not be included in the linting path. I suspect that you are installing roles from outside inside your repo folder and running the linter without excluding that path. Am I wrong?

@rgarrigue
Copy link

@ssbarnea https://github.com/rgarrigue/ansible-role-wikijs

~/personal/ansible-role-wikijs (master ✔) ᐅ rm -rf .tox 
~/personal/ansible-role-wikijs (master ✔) ᐅ tox lint
molecule create: /home/remy/personal/ansible-role-wikijs/.tox/molecule
molecule installdeps: ansible-lint, flake8, molecule[docker]~=3.0, netaddr, requests, testinfra, yamllint
molecule installed: ansible==2.9.12,ansible-lint==4.3.1,appdirs==1.4.3,arrow==0.15.8,attrs==20.1.0,bcrypt==3.2.0,binaryornot==0.4.4,CacheControl==0.12.6,Cerberus==1.3.2,certifi==2019.11.28,cffi==1.14.2,chardet==3.0.4,click==7.1.2,click-completion==0.5.2,click-help-colors==0.8,colorama==0.4.3,commonmark==0.9.1,contextlib2==0.6.0,cookiecutter==1.7.2,cryptography==3.0,distlib==0.3.0,distro==1.4.0,docker==4.3.1,fasteners==0.15,flake8==3.8.3,html5lib==1.0.1,idna==2.8,iniconfig==1.0.1,ipaddr==2.2.0,Jinja2==2.11.2,jinja2-time==0.2.0,lockfile==0.12.2,MarkupSafe==1.1.1,mccabe==0.6.1,molecule==3.0.8,monotonic==1.5,more-itertools==8.4.0,msgpack==0.6.2,netaddr==0.8.0,packaging==20.3,paramiko==2.7.1,pathspec==0.8.0,pep517==0.8.2,pexpect==4.8.0,pluggy==0.13.1,poyo==0.5.0,progress==1.5,ptyprocess==0.6.0,py==1.9.0,pycodestyle==2.6.0,pycparser==2.20,pyflakes==2.2.0,Pygments==2.6.1,PyNaCl==1.4.0,pyparsing==2.4.6,pytest==6.0.1,python-dateutil==2.8.1,python-gilt==1.2.3,python-slugify==4.0.1,pytoml==0.1.21,PyYAML==5.3.1,requests==2.22.0,retrying==1.3.3,rich==5.2.1,ruamel.yaml==0.16.10,ruamel.yaml.clib==0.2.0,selinux==0.2.1,sh==1.13.1,shellingham==1.3.2,six==1.14.0,tabulate==0.8.7,testinfra==5.2.2,text-unidecode==1.3,toml==0.10.1,tree-format==0.1.2,typing-extensions==3.7.4.2,urllib3==1.25.8,webencodings==0.5.1,websocket-client==0.57.0,yamllint==1.24.2
molecule run-test-pre: PYTHONHASHSEED='1751199110'
molecule run-test: commands[0] | molecule lint
--> Test matrix
    
└── default
    ├── dependency
    └── lint
    
--> Scenario: 'default'
--> Action: 'dependency'
    - downloading role 'pip', owned by geerlingguy
    - downloading role from https://github.com/geerlingguy/ansible-role-pip/archive/1.3.0.tar.gz
    - extracting geerlingguy.pip to /home/remy/.cache/molecule/ansible-role-wikijs/default/roles/geerlingguy.pip
    - geerlingguy.pip (1.3.0) was installed successfully
    - downloading role 'postgresql', owned by geerlingguy
    - downloading role from https://github.com/geerlingguy/ansible-role-postgresql/archive/2.2.0.tar.gz
    - extracting geerlingguy.postgresql to /home/remy/.cache/molecule/ansible-role-wikijs/default/roles/geerlingguy.postgresql
    - geerlingguy.postgresql (2.2.0) was installed successfully
Dependency completed successfully.
Skipping, missing the requirements file.
--> Scenario: 'default'
--> Action: 'lint'
--> Executing: set -e
yamllint .
ansible-lint .
flake8
[106] Role name ansible-role-wikijs does not match ``^[a-z][a-z0-9_]+$`` pattern
tasks/main.yml:1
---

[208] File permissions not mentioned
tasks/main.yml:27
Task/Handler: Download sources

You can skip specific rules by adding them to the skip_list section of your configuration file:                                                                                                                     

┌──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│# .ansible-lint                                                                                                                                                                                                   │
│skip_list:                                                                                                                                                                                                        │
│  - '106'  # Role name {} does not match ``^[a-z][a-z0-9_]+$`` pattern'                                                                                                                                           │
│  - '208'  # File permissions not mentioned'                                                                                                                                                                      │
└──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
ERROR: Lint failed: Command 'set -e
yamllint .
ansible-lint .
flake8
' returned non-zero exit status 2.: Command 'set -e
yamllint .
ansible-lint .
flake8
' returned non-zero exit status 2.
ERROR: InvocationError for command /home/remy/personal/ansible-role-wikijs/.tox/molecule/bin/molecule lint (exited with code 1)
_____________________________________________________________________________________________________ summary ______________________________________________________________________________________________________
ERROR:   molecule: commands failed

@ssbarnea
Copy link
Member

@rgarrigue I added #979 which should allow you to avoid this error.

ssbarnea added a commit that referenced this issue Aug 21, 2020
ssbarnea added a commit that referenced this issue Aug 21, 2020
@rgarrigue
Copy link

Interesting, thanks @ssbarnea. Out of curiousity, what's the "new" naming standard ? company.app, app ? Or collections & namespacing ?

jsf9k added a commit to cisagov/skeleton-ansible-role that referenced this issue Aug 21, 2020
ansible-lint now enforces that Ansible role names conform to the
requirements for Ansible collection role names, defined here:
https://docs.ansible.com/ansible/devel/dev_guide/developing_collections.html#roles-directory

Note that this excludes the use of the "-" character, which means that
any Ansible roles using the old naming scheme of ansible-role-* now
trigger an ansible-lint error.  This has caused a lot of woe:
ansible/ansible-lint#966

Fortunately, there is an issue and a corresponding pull request with a
fix:
ansible/ansible-lint#979
ansible/ansible-lint#980

In short, by default ansible-lint assumes that the role name is the
name of the directory containing the role, but one is allowed to
override this behavior by setting the "role_name" entry in the role
metadata.  That will satisfy ansible-lint, and that is what I have
done here.

Also alphabetize the metadata entries by key.
ssbarnea added a commit that referenced this issue Aug 21, 2020
@juju4
Copy link

juju4 commented Aug 22, 2020

First, thanks for the work you do. I can only imagine how hard to enforce a common standard but I think the governance when introducing new lint rules should change.

something like

  • add it in warning mode first
  • at following release, move to error
    depending on how much change it is, may delay more to ensure the whole community is aware of and have some time to implement changes and/or provide feedback. very similar to ansible breaking changes.

else at each new rule, you will likely have thousands of people crying on the project

@nre-ableton
Copy link
Contributor Author

@juju4 Well, ansible-lint does have some notion of rule severity (see the --parseable-severity option), but AFAICS it's not possible to choose which level should be used when running the application. Regardless, I also wish there was a more structured way to introduce new rules. As much as I love and value ansible-lint, I agree that updating it can be a difficult task since new versions typically introduce new rules, which is good.

What is less good is that one must fix all violations before updating to a version, and not afterwards. In short, I wish that ansible-lint had a policy for introducing new rules similar to Ansible's deprecation policy, for example:

[DEPRECATION WARNING]: Invoking "homebrew" only once while using a loop via squash_actions is deprecated. Instead of using a loop to supply multiple items 
and specifying `name: "{{ group_packages_mac_item }}"`, please use `name: '{{ group_packages_mac }}'` and remove the loop. This feature will be removed in 
version 2.11. Deprecation warnings can be disabled by setting deprecation_warnings=False in ansible.cfg.

What works really well about this policy is:

  • My existing playbooks still work, but I get a warning
  • The warning will soon be an error, and I am also told when this will change so I can plan accordingly
  • I can also disable these nags if I like, and I'm told how to do that in the message

I think that ansible-lint would greatly benefit from a similar policy. I'll open up a feature request if I get some support for this idea. Please do so by adding 👍 reaction to this comment.

@hswong3i
Copy link
Contributor

What works really well about this policy is:

* My existing playbooks still work, but I get a warning

* The warning will soon be an error, and I am also told when this will change so I can plan accordingly

* I can also disable these nags if I like, and I'm told how to do that in the message

I think that ansible-lint would greatly benefit from a similar policy. I'll open up a feature request if I get some support for this idea. Please do so by adding +1 reaction to this comment.

Does that means we as ansible-lint should have similar deprecation warning or error message policy as like as Ansible Core itself?

@nre-ableton
Copy link
Contributor Author

Does that means we as ansible-lint should have similar deprecation warning or error message policy as like as Ansible Core itself?

Yes, I am suggesting something similar, though this is a bit in reverse of Ansible Core's policy, because this would be for introducing new rules whereas the Ansible policy is for deprecating old code. But as applied to ansible-lint, I imagine that the policy would look something like this:

  • New rules are initially treated as warnings, meaning that:
    • When caught, they do not cause the program to exit with an error code
    • But, a --strict switch would be added to treat them as errors and exit with a non-zero error code
    • Such warnings would not be shown when ansible-lint is run in --quiet mode
  • New rules would also have a semantic version which they will be treated as errors
  • When bumping ansible-lint's version, someone would have to audit the list of rules and "promote" those which should now be treated as errors

Because this is quite a bit of work, and also involves changes to the ansible-lint release workflow, I'm hesitant to suggest this change outright. But I think that some policy along these lines would make sense.

@ssbarnea
Copy link
Member

@nre-ableton #989 should enable warnings instead of errors.

ssbarnea added a commit that referenced this issue Aug 24, 2020
jsf9k added a commit to cisagov/ansible-role-openvpn that referenced this issue Aug 26, 2020
ansible-lint now enforces that Ansible role names conform to the
requirements for Ansible collection role names, defined here:
https://docs.ansible.com/ansible/devel/dev_guide/developing_collections.html#roles-directory

Note that this excludes the use of the "-" character, which means that
any Ansible roles using the old naming scheme of ansible-role-* now
trigger an ansible-lint error.  This has caused a lot of woe:
ansible/ansible-lint#966

Fortunately, there is an issue and a corresponding pull request with a
fix:
ansible/ansible-lint#979
ansible/ansible-lint#980

In short, by default ansible-lint assumes that the role name is the
name of the directory containing the role, but one is allowed to
override this behavior by setting the "role_name" entry in the role
metadata.  That will satisfy ansible-lint, and that is what I have
done here.

Also alphabetize the metadata entries by key.
stuartgrace-bbc pushed a commit to bbc/ceph-ansible that referenced this issue Jan 30, 2024
Let's pin to 4.2.0

(because of ansible/ansible-lint/issues/966)

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new Triage required
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants