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

Fix role name handling in prerun.py #1490

Merged
merged 13 commits into from Apr 7, 2021
Merged

Fix role name handling in prerun.py #1490

merged 13 commits into from Apr 7, 2021

Conversation

apatard
Copy link
Contributor

@apatard apatard commented Mar 23, 2021

This PR tries to address issues with the link made in .cache/roles for current role name.
The first patch tries to make _install_galaxy_role() behave in same way as ansible-galaxy and
the second one is fixing the link name to use the fqrn computed earlier in the function. This will
provide a way to allow using the role short name by putting the 'role-name' check in
skip_list.

According to
https://galaxy.ansible.com/docs/contributing/creating_role.html#role-metadata:

"role_name is not used at all if the role is installed using its Git URL.
 Instead, the name of the repo is used."

So, make _install_galaxy_role() respect this to compute the role
name. To avoid importing ansible galaxy files, the SCM list
is hardcoded to git or hg.

Signed-off-by: Arnaud Patard <apatard@hupstream.com>
_install_galaxy_role() is taking great pain to get and check
the fqrn, even handling/using the  shortname when the 'role-name'
check is in skip_list but the code is not using the fqrn value to
create the symlink in .cache/roles.
Correct this by using the computed fqrn name for the link name.

Signed-off-by: Arnaud Patard <apatard@hupstream.com>
@apatard apatard marked this pull request as draft March 24, 2021 08:32
Change the ansible-role-mysql git clone path to match the role
fqrn as it's what's used with galaxy and what's currently done
by the role ci.
(ref: https://github.com/geerlingguy/ansible-role-mysql/blob/86ffba892e1553b7e6f042ec007eef25daa8f5db/.github/workflows/ci.yml#L21)

Signed-off-by: Arnaud Patard <apatard@hupstream.com>
@apatard apatard marked this pull request as ready for review March 24, 2021 17:00
playbooks/eco.yml Show resolved Hide resolved
src/ansiblelint/prerun.py Show resolved Hide resolved
src/ansiblelint/prerun.py Outdated Show resolved Hide resolved
@ssbarnea ssbarnea added the bug label Mar 25, 2021
@ssbarnea
Copy link
Member

@geerlingguy Please review this change because it breaks one of your roles. That is because you added, for good reasons, role-name to the skip-list. Stil, the proposed logic is to use compatibility mode (short names) when role-name is excluded and full role names when it is not.

As you can see, with this change we get an error on molecule playbooks as they are unable to find the full qualified role name. Obviously that removing the role-name from the skip list would allow the linter to pass.

Still, I do really want your opinion on this matter as I want to minimize the chance of alienating more users. If you are not happy about it, do you have any proposed behaviors in mind?

If the author field in meta/main.yml looks like "John Doe" (at least 2 words
separated by a space), act as if there was no namespace.
It's not perfect but it's not possible to match author<->galaxy account
and there will still be a warning/error issued about it, since
the fqrn won't match namespace.role_name.

Signed-off-by: Arnaud Patard <apatard@hupstream.com>
According to :
https://galaxy.ansible.com/docs/contributing/creating_role.html#role-metadata:

In the past, Galaxy would apply a regex expression to the GitHub repository
name and automatically remove 'ansible-' and 'ansible-role-'. For example, if
your repository name was 'ansible-role-apache', the role name would translate
to 'apache',

So update the regexp to handle ansible-role- and ansible-

Signed-off-by: Arnaud Patard <apatard@hupstream.com>
If the role-name rule is in skip_list:
- if role_name is set in meta/main.yml, assume that the author wants
  to use this name, even if it's still not up to standards and set
  the link name to the computed fqrn,
- if the role_name is not set, just use the current directory name.
  Note: don't use 'role_name' variable here since some part of the
  name has been stripped from the current directory name.

Signed-off-by: Arnaud Patard <apatard@hupstream.com>
Flake8 is complaining that the function is now too complex, so
split it into several ones.

Signed-off-by: Arnaud Patard <apatard@hupstream.com>
Instead of using None for value not found and sometime return
an empty string make both functions always return a string and
make it empty is something went wrong.

Signed-off-by: Arnaud Patard <apatard@hupstream.com>
@geerlingguy
Copy link

@ssbarnea - TBH, for now I have disabled ansible-lint on all my roles because it was just filling up my inbox every day with another 20+ 'CI failure' notifications and there was an awful lot of work I had to do to each role just to get it to pass lint again (trying to configure ansible-lint so it would skip cache directory—I think that's since been fixed), adding in a list of dependencies in yet another location (the 'mock role' list or something), etc.

In the past, I could just install ansible-lint and it would work, and lint my role. Today it is a lot harder since 5.x came out. I am still using ansible-lint on my playbook repositories, where it seems like very little was changed, but for my Galaxy roles I'm not sure when I'll get the time to work on re-adding configuration for all the roles to run with the linter again.

@apatard
Copy link
Contributor Author

apatard commented Mar 30, 2021

@ssbarnea - TBH, for now I have disabled ansible-lint on all my roles because it was just filling up my inbox every day with another 20+ 'CI failure' notifications and there was an awful lot of work I had to do to each role just to get it to pass lint again (trying to configure ansible-lint so it would skip cache directory—I think that's since been fixed), adding in a list of dependencies in yet another location (the 'mock role' list or something), etc.

The cache thing should be fixed iirc. For the dependency location thing, it's a different issue and if I find time I'll probably look at that too as I got bitten by it too, but in a different PR. No idea what @ssbarnea will think of that.

In the past, I could just install ansible-lint and it would work, and lint my role. Today it is a lot harder since 5.x came out. I am still using ansible-lint on my playbook repositories, where it seems like very little was changed, but for my Galaxy roles I'm not sure when I'll get the time to work on re-adding configuration for all the roles to run with the linter again.

With this PR, I hope that for roles without dependencies, things should just work again, well except for roles with wrong fqrn and in this case, either fixing or .ansible-lint is the solution. With my latest changes, ansible-lint is happy with your role but still having feedback from you on how it should work is interesting imho.

apatard and others added 3 commits March 30, 2021 17:41
As it may lead to breakages, drop the check on .git or .hg
presence to get role name.

Signed-off-by: Arnaud Patard <apatard@hupstream.com>
@ssbarnea ssbarnea merged commit 38a5dea into ansible:master Apr 7, 2021
pwalczysko added a commit to pwalczysko/ansible-role-omero-server that referenced this pull request Jul 24, 2023
pwalczysko added a commit to pwalczysko/ansible-role-rsync-server that referenced this pull request Jul 24, 2023
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 this pull request may close these issues.

None yet

3 participants