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 bypassing role name checking #3549

Merged
merged 1 commit into from
Jun 1, 2022
Merged

Allow bypassing role name checking #3549

merged 1 commit into from
Jun 1, 2022

Conversation

zhan9san
Copy link
Contributor

@zhan9san zhan9san marked this pull request as draft May 24, 2022 09:34
@zhan9san
Copy link
Contributor Author

Hi @briantist

Could you whether this PR address your issue?

Ensure role_name_check: 1 is added in molecule.yml file if the role name is invalid. like below

https://github.com/ansible-community/molecule/pull/3549/files#diff-6d870a34c1531cdde908908cf22ee6b5957ca848266004cb1e5b39c679a27a50

@zhan9san zhan9san marked this pull request as ready for review May 24, 2022 10:14
@ssbarnea ssbarnea changed the title support role name check Allow bypassing role name checking May 24, 2022
@ssbarnea
Copy link
Member

This PR looks ok to me but I want confirmation from someone else that it is fixing that issue.

@Xiol
Copy link

Xiol commented May 24, 2022

I can't comment on if this PR itself is fixing the problem as I don't have a way to test it, but setting role_name_check=2 as the option to self._install_galaxy_role in ansible-compat's runtime.py file does resolve the problem I've been having with this (there is some abuse of sed going on 👀)

As far as I can tell this PR will set the same value, so I would think this will solve the problem.

(Using '2' as the value does mean you have to use the fully-qualified role name in converge.yml, I'm not sure what the behaviour is like with '1').

@zhan9san
Copy link
Contributor Author

@Xiol

How to verify this PR

pip install git+https://github.com/zhan9san/molecule@feature/support-role-name-check

If role name is invalid, such as foo-bar which contains a invalid character, dash.

Run test without role_name_check configuration item.

  1. Run test
molecule test
  1. Expected result

    Failed

Run test with role_name_check: 1 configuration item.

Ensure role_name_check: 1 is added in molecule.yml file.

  1. Run test
molecule test
  1. Expected result

    Successful

Note: It is not molecule's responsibility to fix ansible-lint issues, so you have to fix/skip them by yourself.

@briantist
Copy link

@zhan9san thank you, I definitely would like to try this out, this week is very busy for me and I'll be away from the computer much more than usual, so I may not be able to try it for a few days.

@ssbarnea
Copy link
Member

ssbarnea commented Jun 1, 2022

@zhan9san Can you also rebase this, so I can include it in release? Thanks.

@zhan9san zhan9san requested review from a team as code owners June 1, 2022 08:44
@zhan9san zhan9san requested review from cloudnull and ganeshrn and removed request for a team June 1, 2022 08:44
@zhan9san
Copy link
Contributor Author

zhan9san commented Jun 1, 2022

@ssbarnea

It is rebased.

Please take a look.

@ssbarnea ssbarnea merged commit d263e98 into ansible:main Jun 1, 2022
@geerlingguy
Copy link
Contributor

So if I'm reading this right, I should set:

role_name_check: 1

In my molecule.yml file (e.g. this one), and then the role name check should finally be skipped and I won't be getting:

ERROR    Computed fully qualified role name of geerlingguy.php-mysql does not follow current galaxy requirements.
...
As an alternative, you can add 'role-name' to either skip_list or warn_list.

Should I still keep role-name in my lint file skip_list too?

@zhan9san
Copy link
Contributor Author

zhan9san commented Jun 1, 2022

Yes.
You can set role_name_check: 1 in molecule.yml, and then the invalid name format issue would be skipped.

And there is no need to keep role-name in lint file skip_list.

Does it work for you?

@geerlingguy
Copy link
Contributor

Right now I'm not setup to test on the bleeding edge release but may try it out in a bit. Do you know when the next stable release will hit pip? At that point it's just a matter of waiting until my CI runs again.

@zhan9san
Copy link
Contributor Author

zhan9san commented Jun 2, 2022

I am afraid I can't give you an exact date.

@ssbarnea
Copy link
Member

ssbarnea commented Jun 2, 2022

@geerlingguy I am sure you know how to use --pre flag on pip installs or 'ansible-lint>=4.0.0a0' to get the newer version. We made a pre-release for v4, so we can get feedback without asking people to install from main branch, which is bit harder.

@geerlingguy
Copy link
Contributor

geerlingguy commented Jun 2, 2022

I'm still getting a WARNING in the output:

 14:25:48 VMs/roles/geerlingguy.php-mysql 
$ molecule --version
molecule 4.0.0a0 using python 3.9 
    ansible:2.12.6
    delegated:4.0.0a0 from molecule
    docker:1.1.0 from molecule_docker requiring collections: community.docker>=1.9.1

 14:26:06 VMs/roles/geerlingguy.php-mysql 
$ molecule test
INFO     default scenario test matrix: dependency, lint, cleanup, destroy, syntax, create, prepare, converge, idempotence, side_effect, verify, cleanup, destroy
INFO     Performing prerun with role_name_check=1...
INFO     Set ANSIBLE_LIBRARY=/Users/jgeerling/.cache/ansible-compat/24324c/modules:/Users/jgeerling/.ansible/plugins/modules:/usr/share/ansible/plugins/modules
INFO     Set ANSIBLE_COLLECTIONS_PATH=/Users/jgeerling/.cache/ansible-compat/24324c/collections:/Users/jgeerling/.ansible/collections:/usr/share/ansible/collections
INFO     Set ANSIBLE_ROLES_PATH=/Users/jgeerling/.cache/ansible-compat/24324c/roles:/Users/jgeerling/Dropbox/VMs/roles
WARNING  Computed fully qualified role name of geerlingguy.php-mysql does not follow current galaxy requirements.
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

As an alternative, you can add 'role-name' to either skip_list or warn_list.

INFO     Using /Users/jgeerling/.cache/ansible-compat/24324c/roles/geerlingguy.php-mysql symlink to current repository in order to enable Ansible to find the role using its expected full name.
INFO     Running default > dependency
Starting galaxy role install process
- downloading role 'repo-remi', owned by geerlingguy
- downloading role from https://github.com/geerlingguy/ansible-role-repo-remi/archive/2.0.1.tar.gz
- extracting geerlingguy.repo-remi to /Users/jgeerling/.cache/molecule/geerlingguy.php-mysql/default/roles/geerlingguy.repo-remi
- geerlingguy.repo-remi (2.0.1) was installed successfully
- downloading role 'apache', owned by geerlingguy
- downloading role from https://github.com/geerlingguy/ansible-role-apache/archive/3.3.0.tar.gz
- extracting geerlingguy.apache to /Users/jgeerling/.cache/molecule/geerlingguy.php-mysql/default/roles/geerlingguy.apache
- geerlingguy.apache (3.3.0) was installed successfully
- downloading role 'mysql', owned by geerlingguy
- downloading role from https://github.com/geerlingguy/ansible-role-mysql/archive/3.5.0.tar.gz
- extracting geerlingguy.mysql to /Users/jgeerling/.cache/molecule/geerlingguy.php-mysql/default/roles/geerlingguy.mysql
- geerlingguy.mysql (3.5.0) was installed successfully
- downloading role 'php', owned by geerlingguy
- downloading role from https://github.com/geerlingguy/ansible-role-php/archive/4.8.0.tar.gz
- extracting geerlingguy.php to /Users/jgeerling/.cache/molecule/geerlingguy.php-mysql/default/roles/geerlingguy.php
- geerlingguy.php (4.8.0) was installed successfully
INFO     Dependency completed successfully.
WARNING  Skipping, missing the requirements file.
INFO     Running default > lint
INFO     Lint is disabled.
INFO     Running default > cleanup

The role I'm testing with (at the latest commit): https://github.com/geerlingguy/ansible-role-php-mysql

role-name is in the skip list: https://github.com/geerlingguy/ansible-role-php-mysql/blob/master/.ansible-lint#L3

And role_name_check: 1 is in the molecule.yml: https://github.com/geerlingguy/ansible-role-php-mysql/blob/master/molecule/default/molecule.yml#L2

It seems like the test at least progresses beyond that warning now (previously it was an ERROR and stopped execution), but if I have it configured to be skipped entirely, it's weird it still spits out that warning.

@Xiol
Copy link

Xiol commented Jun 2, 2022

I think that's expected when you set it to 1. Try setting it to 2 - I believe that suppresses the warning.

@geerlingguy
Copy link
Contributor

geerlingguy commented Jun 2, 2022

Ah, right you are! Updating that now.

Edit: Works with 2.

@zhan9san
Copy link
Contributor Author

zhan9san commented Jun 3, 2022

If it's set to 1, we are trying to remind the user this is still not a recommended way.

Besides, it does not block users.

@askb
Copy link

askb commented Jun 29, 2022

Does this requires using a more recent version of ansible > 2.9.6 ? Seem to be hitting the issue even of setting the role_name: namespace: and role_name_check: 1 with the latest version of molecule.

10:44:01 molecule create: /w/workspace/lf-infra-ansible-roles-tox-verify-any/.tox/molecule
10:44:02 molecule installdeps: ansible~=2.9.6, ansible-lint~=4.2.0, detox~=0.18, docker, yamllint, molecule, molecule[docker], testinfra>=3.0.0, pytest~=5.4.0
10:44:37 molecule installed: ansible==2.9.27,ansible-compat==1.0.0,ansible-lint==4.2.0,arrow==1.2.2,attrs==21.4.0,bcrypt==3.2.2,binaryornot==0.4.4,cached-property==1.5.2,Cerberus==1.3.2,certifi==2022.6.15,cffi==1.15.0,chardet==5.0.0,charset-normalizer==2.0.12,click==8.0.4,click-help-colors==0.9.1,commonmark==0.9.1,cookiecutter==1.7.3,cryptography==37.0.3,dataclasses==0.8,detox==0.19,distlib==0.3.4,distro==1.7.0,dnspython==2.2.1,docker==5.0.3,enrich==1.2.7,eventlet==0.33.1,filelock==3.4.1,greenlet==1.1.2,idna==3.3,importlib-metadata==4.8.3,importlib-resources==5.4.0,Jinja2==3.0.3,jinja2-time==0.2.0,MarkupSafe==2.0.1,molecule==3.6.1,molecule-docker==1.1.0,more-itertools==8.13.0,packaging==21.3,paramiko==2.11.0,pathspec==0.9.0,platformdirs==2.4.0,pluggy==0.13.1,poyo==0.5.0,py==1.11.0,pycparser==2.21,Pygments==2.12.0,PyNaCl==1.5.0,pyparsing==3.0.9,pytest==5.4.3,pytest-testinfra==6.8.0,python-dateutil==2.8.2,python-slugify==6.1.2,PyYAML==6.0,requests==2.27.1,rich==12.4.4,ruamel.yaml==0.17.21,ruamel.yaml.clib==0.2.6,selinux==0.2.1,six==1.16.0,subprocess-tee==0.3.5,testinfra==6.0.0,text-unidecode==1.3,toml==0.10.2,tox==3.6.1,typing_extensions==4.1.1,urllib3==1.26.9,virtualenv==20.15.0,wcwidth==0.2.5,websocket-client==1.3.1,yamllint==1.26.3,zipp==3.6.0
10:44:37 molecule run-test-pre: PYTHONHASHSEED='3979270437'
10:44:37 molecule run-test: commands[0] | ./molecule.sh
10:44:38 /w/workspace/lf-infra-ansible-roles-tox-verify-any/.tox/molecule/lib/python3.6/site-packages/ansible/parsing/vault/__init__.py:44: CryptographyDeprecationWarning: Python 3.6 is no longer supported by the Python core team. Therefore, support for it is deprecated in cryptography and will be removed in a future release.
10:44:38   from cryptography.exceptions import InvalidSignature
10:44:38 /w/workspace/lf-infra-ansible-roles-tox-verify-any/.tox/molecule/lib/python3.6/site-packages/requests/__init__.py:104: RequestsDependencyWarning: urllib3 (1.26.9) or chardet (5.0.0)/charset_normalizer (2.0.12) doesn't match a supported version!
10:44:38   RequestsDependencyWarning)
10:44:38 INFO     default scenario test matrix: dependency, lint, cleanup, destroy, syntax, create, prepare, converge, idempotence, side_effect, verify, cleanup, destroy
10:44:38 INFO     Performing prerun...
10:44:38 INFO     Set ANSIBLE_LIBRARY=/home/jenkins/.cache/ansible-compat/e97c04/modules:/home/jenkins/.ansible/plugins/modules:/usr/share/ansible/plugins/modules
10:44:38 INFO     Set ANSIBLE_COLLECTIONS_PATHS=/home/jenkins/.cache/ansible-compat/e97c04/collections:/home/jenkins/.ansible/collections:/usr/share/ansible/collections
10:44:38 INFO     Set ANSIBLE_ROLES_PATH=/home/jenkins/.cache/ansible-compat/e97c04/roles:/home/jenkins/.ansible/roles:/usr/share/ansible/roles:/etc/ansible/roles
10:44:38 ERROR    Computed fully qualified role name of lfit.ansible-galaxy-puppet-install does not follow current galaxy requirements.
10:44:38 Please edit meta/main.yml and assure we can correctly determine full role name:
10:44:38 
10:44:38 galaxy_info:
10:44:38 role_name: my_name  # if absent directory name hosting role is used instead
10:44:38 namespace: my_galaxy_namespace  # if absent, author is used instead
10:44:38 
10:44:38 Namespace: https://galaxy.ansible.com/docs/contributing/namespaces.html#galaxy-namespace-limitations
10:44:38 Role: https://galaxy.ansible.com/docs/contributing/creating_role.html#role-names
10:44:38 
10:44:38 As an alternative, you can add 'role-name' to either skip_list or warn_list.

@zhan9san
Copy link
Contributor Author

@askb

This feature is included in molecule==4.0.0. But I noticed molecule==3.6.1 is installed.

Could you update the molecule and try again?

@askb
Copy link

askb commented Jun 30, 2022

@zhan9san Not sure what is pulling the older version of molecule, since its not pinned int tox.ini. Does this require more recent version of ansible-lint or ansible?

[testenv:molecule]
basepython = python3
deps =
    ansible~=2.9.6
    ansible-lint~=4.2.0
    detox~=0.18
    docker
    yamllint
    molecule
    molecule[docker]
    testinfra>=3.0.0
    pytest~=5.4.0

https://jenkins.opendaylight.org/releng/job/lf-infra-ansible-roles-tox-verify-any/668/console

@zhan9san
Copy link
Contributor Author

@askb

Neither ansible-lint nor ansible requires molecule.

We cannot identify the dependency tree from Jenkins log.

Could you list the dependencies via pipdeptree?

@zhan9san zhan9san deleted the feature/support-role-name-check branch June 30, 2022 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'role-name' is in skip_list, but is not being skipped
6 participants