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

Ensure roles have a namespace when created #3277

Merged
merged 1 commit into from Nov 25, 2021

Conversation

ssbarnea
Copy link
Member

@ssbarnea ssbarnea commented Oct 25, 2021

As test tools require the presence of a namespace, we now detect when user is trying to create standalone roles and require him to mention the namespace.

This also updates the command line help text to provide better information regarding what is expected from the user.

This change does not affect creation of new roles within collections.

Related: ansible/ansible-compat#78

@ssbarnea ssbarnea added the bug label Oct 25, 2021
@ssbarnea ssbarnea force-pushed the fix/role-name branch 3 times, most recently from 47e4a83 to 01a443a Compare October 25, 2021 10:13
@ssbarnea ssbarnea marked this pull request as ready for review October 25, 2021 10:37
As test tools require the presence of a namespace, we now detect when
user is trying to create standalone roles and require him to mention
the namespace.

Related: ansible/ansible-compat#78

# outside collections our tooling needs a namespace.
if not os.path.isfile("../galaxy.yml"):
name_re = re.compile(r"^[a-z][a-z0-9_]+\.[a-z][a-z0-9_]+$")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you have this regex here already, why not declare groups for the namespace and the role and just extract them from match() instead of re-splitting later?

if not os.path.isfile("../galaxy.yml"):
name_re = re.compile(r"^[a-z][a-z0-9_]+\.[a-z][a-z0-9_]+$")

if not name_re.match(role_name):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, note the ansible-core has a more precise check that you may want to reuse: https://github.com/ansible/ansible/pull/73279/files#diff-3d1b69b541811ef19d2c1ede9acd53ed73c6cafe37b787c3ce209357279c6c01R871-R876. It doesn't use regex directly but a check for something being a valid Python identifier.
Plus, we could just reuse that method. It exists since ansible v2.9.

Suggested change
if not name_re.match(role_name):
from ansible.utils.collection_loader import AnsibleCollectionRef
if not AnsibleCollectionRef.is_valid_fqcr(role_name):

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will merge that like this because fixing that bug is quite important. My plan was to add code to ansible-compat for doing this but that may take extra time. I will look into replacing this code with the one from compat, once we have it.

@ssbarnea ssbarnea merged commit c0274bb into ansible:main Nov 25, 2021
@ssbarnea ssbarnea deleted the fix/role-name branch November 25, 2021 09:14
@ssbarnea
Copy link
Member Author

Note: I plan to address this in follow-up.

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

2 participants