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

role_name check is not skipped when autodetecting standalone role name #1454

Closed
geerlingguy opened this issue Mar 10, 2021 · 13 comments · Fixed by #1459
Closed

role_name check is not skipped when autodetecting standalone role name #1454

geerlingguy opened this issue Mar 10, 2021 · 13 comments · Fixed by #1459
Labels

Comments

@geerlingguy
Copy link

Summary

5.0.3 is sooo close to working with all my roles again, but I've run into this issue:

I have a role named geerlingguy.apache-php-fpm. It is on:

When I run ansible-lint I get:

Computed fully qualified role name of geerlingguy.apache-php-fpm is not valid.
Please edit meta/main.yml and assure we can correctly determine full role name:

galaxy_info:
  role_name: my_name  # if absent directory name hosting role is used instead
  namespace: my_galaxy_namespace  # if absent, author is used instead

Namespace: https://galaxy.ansible.com/docs/contributing/namespaces.html#galaxy-namespace-limitations
Role: https://galaxy.ansible.com/docs/contributing/creating_role.html#role-names

Error: Process completed with exit code 10.

I even have, inside my .ansible-lint file:

skip_list:
  - 'role-name'

But it seems like the computed role name is run against the role-name linter before Ansible Lint even starts linting the role`. I think, instead, that the role-name lint should not be run as part of setup at all, since roles can have dashes (especially roles that are not on galaxy itself—I hate Ansible's war on dashes of late, and use them extensively in roles, group names, etc.).

See failed run: https://github.com/geerlingguy/ansible-role-apache-php-fpm/runs/2080256861?check_suite_focus=true

Issue Type
  • Bug Report
Ansible and Ansible Lint details
ansible 2.10.5
  config file = /etc/ansible/ansible.cfg
  configured module search path = ['/Users/jgeerling/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/local/lib/python3.9/site-packages/ansible
  executable location = /usr/local/bin/ansible
  python version = 3.9.2 (default, Feb 24 2021, 13:26:09) [Clang 12.0.0 (clang-1200.0.32.29)]

ansible-lint 5.0.3 using ansible 2.10.5
  • ansible installation method: pip
  • ansible-lint installation method: pip
OS / ENVIRONMENT

macOS, Ubuntu (GitHub Actions CI)

STEPS TO REPRODUCE

Run ansible-lint on my apache-php-fpm role on GitHub: https://github.com/geerlingguy/ansible-role-apache-php-fpm

Desired Behaviour

Ansible Lint passes.

Actual Behaviour

Ansible Lint fails with error message above.

@ssbarnea
Copy link
Member

I am afraid that based on galaxy rules, you cannot used dashes. Use only allowed chars in role_name and namespace.

@geerlingguy
Copy link
Author

@ssbarnea - But Ansible Galaxy has explicitly allowed this for roles that were created prior to the 'great dash purge' a year or so ago... case in point, all my roles are still listed and importing correctly.

See galaxy bug report and fix: ansible/galaxy#2622 (comment)

So yes, it is still good to do that lint check (for new roles)... but what I'm proposing is it gets removed from the hardcoded pre-flight checks. Because otherwise I can't skip it using the skip_list like I'm already trying to do...

@ssbarnea
Copy link
Member

What happens when you install such a role? How can you access such a role? If galaxy does sanitize the role name we could try to replicate that behaviour. The scope of the pre-run step is to simulate installation from galaxy, so we can lint the entire repository (including test playbooks that may need to import the role using its full name).

@geerlingguy
Copy link
Author

geerlingguy commented Mar 10, 2021

Galaxy does not. I install it with ansible-galaxy install geerlingguy.apache-php-fpm. It installs fine. In my playbooks I refer to the role as geerlingguy.apache-php-fpm and it works just fine.

I just re-ran the import of the role on Galaxy, too, and there were no errors whatsoever:

Starting import: task_id=789075, repository=geerlingguy/ansible-role-apache-php-fpm 
 
===== LOADING ROLE ===== 
Checking role metadata tags 
Checking role platforms 
Checking role cloud platforms 
Checking role dependencies 
 
===== LINTING ROLE: apache-php-fpm ===== 
yamllint OK. 
ansible-lint OK. 
 
===== IMPORTING ROLE: apache-php-fpm ===== 
 
Updating repository versions... 
Import completed with 0 warnings and 0 errors 

@ssbarnea
Copy link
Member

Docs say that galaxy replaces dashes with underlines... https://galaxy.ansible.com/docs/contributing/creating_role.html

So what happens in practice is not what is documented?

@geerlingguy
Copy link
Author

geerlingguy commented Mar 10, 2021

New roles are not allowed to have dashes. Existing roles are. That seems to be a bug in the docs, fixed by ansible/galaxy#2642

I don't write the rules, I just complain about breaking changes :D

@ssbarnea
Copy link
Member

@awcrosby Can you please advise here? Clearly galaxy created some kind of mess in that area, with new rules that do not even apply equally.

Maybe setting a fixed date for performing an automatic sanitisation of existing names would not be the most insane approach? Some would break, but at least we can move on.

@awcrosby
Copy link
Contributor

Maybe the best way to summarize it, is that galaxy code recommends standalone role names have no dashes, and enforces it for new standalone roles.

It sounds like you want ansible-lint to take a hard stance on one or the other, but I’m not sure we want to change galaxy to take a hard stance at this point, especially since we recommend folks creating new roles have them inside a collection. Can ansible-lint handle that as a more of a "recommendation"?

@ssbarnea
Copy link
Member

That one is tricky because it happens before we even run the linting process, in what is currently name "pre-run", where we install dependencies.

I would have to do some refactoring for making this failure "soft" and allow the process to continue, as you said there are good chances the linting would succeed after. Once I do this I can include that new prerun-invalid-role-name to the default warn_list.

I would also update the message displayed and assure that it will mention that this means that linting may fail later with ansible failing to find the role.

@awcrosby @geerlingguy Does this sound like an acceptable behavior? You will likely get an warning, but the linting should be passing for your "old-style" standalone role.

@geerlingguy
Copy link
Author

That's okay IMO... but note that role names with dashes are still perfectly valid (and used often) outside of Galaxy (with just plain old ansible, and via direct installation from Git repos)... so I'd prefer the role name lint to be not applied (or modified to allow dashes) as a preflight check at all.

@ssbarnea
Copy link
Member

Considering that I already see the role-name inside your skip_list, I am inclined to rely on it and re-use it. I can do the check only if `role-name' is not disabled by the user. Probably this would be better as those like you are likely to already have it and not want to bother about it.

@awcrosby
Copy link
Contributor

Sounds like that way a new standalone role author would get the warning, which could be helpful. Speaking from a galaxy perspective, this sounds acceptable.

ssbarnea added a commit that referenced this issue Mar 11, 2021
- when `role-name` is added to skip_list, just install simple role name
- when `role-name` is added to warn_list, display warning and continue
  execution.

Fixes: #1454
ssbarnea added a commit that referenced this issue Mar 11, 2021
- when `role-name` is added to skip_list, just install simple role name
- when `role-name` is added to warn_list, display warning and continue
  execution.

Fixes: #1454
@geerlingguy
Copy link
Author

Probably this would be better as those like you are likely to already have it and not want to bother about it.

That sounds perfect! That way new users would still get the fail, but old curmudgeons with grandfathered role names will be able to work around it.

ssbarnea added a commit that referenced this issue Mar 12, 2021
- when `role-name` is added to skip_list, just install simple role name
- when `role-name` is added to warn_list, display warning and continue
  execution.

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

Successfully merging a pull request may close this issue.

3 participants