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
Fixed/improved regular expresssion for collection names #73577
Conversation
Related #73279 |
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 change looks correct at first glance, but it needs tests and a changelog fragment. See this fragment as an example.
@samdoran changelog fragment added. |
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.
The changelog looks good. Can you please add some more tests cases to the existing unit tests?
ansible/test/units/utils/collection_loader/test_collection_loader.py
Lines 724 to 735 in f327e65
('ns1.coll2', True), | |
('def.coll3', False), | |
('ns4.return', False), | |
('assert.this', False), | |
('import.that', False), | |
('.that', False), | |
('this.', False), | |
('.', False), | |
('', False), | |
), | |
) | |
def test_fqcn_validation(fqcn, expected): |
ansible/test/units/utils/collection_loader/test_collection_loader.py
Lines 743 to 747 in f327e65
('no_dots_at_all_action', 'action', ValueError, 'is not a valid collection reference'), | |
('no_nscoll.myaction', 'action', ValueError, 'is not a valid collection reference'), | |
('ns.coll.myaction', 'bogus', ValueError, 'invalid collection ref_type'), | |
]) | |
def test_fqcr_parsing_invalid(ref, ref_type, expected_error_type, expected_error_expression): |
I love the smell of rebase in the morning |
@samdoran added a couple of lines to the test data - not a big change so not much to add there. |
SUMMARY
The regexp to determine a collection name in
schema.py
:has two bugs: a) it does not protect the dot between names, allowing things like
community#general
orcommunity%general
or evencommunitygeneral
to be validated, when they shouldn't, and b) it fails to validate namespaces with deeper directory structures.While investigating this, simplified a regexp in
lib/ansible/utils/collection_loader/_collection_finder.py
as wellISSUE TYPE
COMPONENT NAME
lib/ansible/utils/collection_loader/_collection_finder.py
test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/schema.py