-
Notifications
You must be signed in to change notification settings - Fork 659
fix: misleading documentation on role name restrictions #2420
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
Open
gardar
wants to merge
1
commit into
ansible:devel
Choose a base branch
from
gardar:fix-role-name-underscore
base: devel
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -168,7 +168,7 @@ roles directory | |||||
|
||||||
Collection roles are mostly the same as existing roles, but with a couple of limitations: | ||||||
|
||||||
- Role names are now limited to contain only lowercase alphanumeric characters, plus ``_`` and start with an alpha character. | ||||||
- Role names can only contain lowercase alphanumeric characters and underscores ( ``_`` ). Role names prefixed with an underscore are not intended for direct user invocation and should be reserved for internal collection roles only. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are also spaces around the underscore in the brackets which should be removed. |
||||||
- Roles in a collection cannot contain plugins any more. Plugins must live in the collection ``plugins`` directory tree. Each plugin is accessible to all roles in the collection. | ||||||
|
||||||
The directory name of the role is used as the role name. Therefore, the directory name must comply with the above role name rules. The collection import into Galaxy will fail if a role name does not comply with these rules. | ||||||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still misleading, since it notes an external limitation imposed by Galaxy without the necessary level of detail to understand the limitation. Ansible itself only requires that the role name be a valid Python identifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I'm assuming you are talking about this in particular:
Do you want me to rephrase it or perhaps just remove it altogether?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two things that should be documented. The first is that collections impose additional restrictions on role names, by requiring that they be valid Python identifiers [0]. The second is that Galaxy imposes further restrictions on role names, by enforcing a more limited character set.
[0] Actually, I appear to be confident but wrong here. I expected it to fail on a role named
2
but it works just fine, which leaves me with no idea of what the real restrictions are.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to https://github.com/ansible/ansible/blob/f11dfa7cce0f939a5dc1e11addc6cfbb5c7fe030/lib/ansible/utils/collection_loader/_collection_finder.py#L916 it's likely this:
According to https://docs.python.org/3/library/re.html,
\w
covers:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would separate this into several 'sub bullets', otherwise it is either too long or non substantive enough:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separating it into three "sub bullets" is a good idea
Just to confirm, we are certain that these are the two real restrictions?