Including file type extension #294
Conversation
CHANGES/6223.bugfix
Outdated
| @@ -0,0 +1 @@ | |||
| Including file type extension | |||
There was a problem hiding this comment.
Can this call out the upload case as when the user would experience it?
pulp_ansible/app/galaxy/views.py
Outdated
| @@ -157,8 +157,10 @@ def post(self, request, path): | |||
| artifact = Artifact.init_and_validate(serializer.validated_data["file"]) | |||
| artifact.save() | |||
|
|
|||
| filename = serializer.validated_data["file"].name | |||
There was a problem hiding this comment.
One of the concerns is about falsified content so we can't rely on this user provided data. We can rely that the name should always be namespace-name-version.tar.gz like the example here https://docs.ansible.com/ansible/latest/user_guide/collections_using.html#id2
Maybe we need another helper function like this one? https://github.com/pulp/pulp_ansible/blob/master/pulp_ansible/app/models.py#L166 but for the filename generation.
There was a problem hiding this comment.
the extension will always be tar.gz?
| @@ -169,9 +171,7 @@ def import_collection( | |||
| collection_version.save() # Save the FK updates | |||
|
|
|||
| ContentArtifact.objects.create( | |||
| artifact=artifact, | |||
| content=collection_version, | |||
| relative_path=collection_version.relative_path, | |||
There was a problem hiding this comment.
if it is always tar.gz
we only have to do:
relative_path=f"{collection_version.relative_path}.tar.gz",
There was a problem hiding this comment.
That's the solution I had in mind rather that using the filename of the file uploaded by the user.
There was a problem hiding this comment.
Also, the relative_path property on CollectionVersion is only used here. Seems like we should probably update it.
There was a problem hiding this comment.
I was also thinking this. To check if it's safe is it used elsewhere in the code at all? Also does https://github.com/ansible/galaxy-dev/ or https://github.com/ansible/galaxy-importer/ use it? If not then I agree completely.
There was a problem hiding this comment.
Good question. I don't see it when I do a code search.
There was a problem hiding this comment.
the log above is a stderr for ansible-galaxy, all the 3 tests that used it failed
There was a problem hiding this comment.
The failing tests are because this only adds the file extension on uploaded collection versions here. It also needs to be added to synced collection versions or else they will also be missing their file extensions too. Once I did that, I was able to set the file extension on the relative_path property and all the tests are passing now: #295.
There was a problem hiding this comment.
should I close this PR in favor of #295 ?
There was a problem hiding this comment.
No, we still need the migration. I was hoping you could work on that and I could just close out #295 ?
CHANGES/6223.bugfix
Outdated
| @@ -0,0 +1 @@ | |||
| Including file type extension when uploading collections | |||
There was a problem hiding this comment.
Can this also indicate that it fixes the user's busted database with a data migration.
https://pulp.plan.io/issues/6223
closes #6223