Build collections URL according to requirements.yml #353
Conversation
|
Attached issue: https://pulp.plan.io/issues/7412 |
|
@fao89 I looked at this problem and I think we should do something like this: https://pulp.plan.io/issues/7412#note-5 instead. wdyt? |
pulp_ansible/app/serializers.py
Outdated
| result = re.findall(r"/v(\d)/collections", url) | ||
| if not result: | ||
| raise serializers.ValidationError( | ||
| _("Invalid URL {url}".format(url=data["url"])) |
There was a problem hiding this comment.
I couldn't think a better error message
There was a problem hiding this comment.
Maybe Inavlid URL {url}. Ensure the URL ends in either '/v2/collections/' or '/v3/collections/'.
this is much better now, thank you! |
| @@ -132,7 +133,18 @@ def validate(self, data): | |||
| data = super().validate(data) | |||
|
|
|||
| if data.get("requirements_file"): | |||
| parse_collections_requirements_file(data["requirements_file"]) | |||
| parsed_file = parse_collections_requirements_file(data["requirements_file"]) | |||
There was a problem hiding this comment.
I'm confused, I didn't think we would be validating the requirements file since it doesn't contain urls typically. https://docs.ansible.com/ansible/devel/galaxy/user_guide.html#install-multiple-collections-with-a-requirements-file
I thought when a requirements file is being used, the remote.url would be validated.
There was a problem hiding this comment.
I'm validating source which is a URL
| @@ -82,7 +82,7 @@ | |||
| collections: | |||
| - name: testing.ansible_testing_content | |||
| version: ">=1.0.0,<=2.0.0" | |||
| source: https://galaxy-dev.ansible.com | |||
| source: https://galaxy-dev.ansible.com/api/v2/collections | |||
There was a problem hiding this comment.
@bmbouter see this URL would break
| @@ -324,7 +324,7 @@ def _get_url(page, api_version): | |||
| name, version, source = collection_info[page - 1] | |||
| namespace, name = name.split(".") | |||
| root = source or remote.url | |||
| url = f"{root}/api/v2/collections/{namespace}/{name}" | |||
| url = f"{root}/{namespace}/{name}" | |||
There was a problem hiding this comment.
it picks source or the remote.url, so I had to validate it
There was a problem hiding this comment.
I see how this correctly handles the case of forming the url for the requirements file, but in the case of no requirements file, where does it form those urls from?
There was a problem hiding this comment.
if it does not have a requirements file, it does not fall on this if
There was a problem hiding this comment.
Cool, so to confirm, we are able to form the right urls when not using a requirements file?
There was a problem hiding this comment.
yes, we are, we have lots of tests doing that
pulp_ansible/app/serializers.py
Outdated
| urls_to_validate.append(source) | ||
|
|
||
| for url in urls_to_validate: | ||
| result = re.findall(r"/v(\d)/collections", url) |
There was a problem hiding this comment.
What do we do about the optional trailing slash or not? I think we either need to require it or not, but not allow both.
There was a problem hiding this comment.
Also what about cases where it's something like /something/api/v2/collection/more/stuff/v2/collectionmine/. This would also validate. So I think validating at the end is key.
There was a problem hiding this comment.
So maybe this check would instead be:
if url.endswith('/v2/collections/) or url.endswith('/v3/collections/):
or possibly if url.endswith('/v2/collections) or url.endswith('/v3/collections')` depending on how the trailing slash is handled.
https://pulp.plan.io/issues/7412
closes #7412