Refactor CollectionVersion Upload to use pulpcore machinery #1176

gerrod3 · 2022-08-17T22:08:39Z

pulp_ansible/app/galaxy/views.py

pulp_ansible/app/serializers.py

mdellweg · 2022-08-19T08:24:45Z

pulp_ansible/tests/functional/api/collection/v3/test_collection.py

@@ -309,27 +309,17 @@ def test_collection_download(artifact, pulp_client, collection_detail):
    assert f.content == tarball


-def test_collection_upload_repeat(pulp_client, known_collection, pulp_dist):
+def test_collection_upload_repeat(pulp_client, artifact, pulp_dist, collection_upload):


what kind of a fixture is "artifact"? If it's a pulp_ansible one can we name it collection_artifact?

Are the changes in this file supposed to be part of this PR at all?

Yes, I realized the tests for collection upload were wrong and incorrectly passing when it shouldn't.

mdellweg · 2022-08-19T08:26:21Z

pulp_ansible/tests/functional/api/collection/v3/test_collection.py

+    # TODO: Add this back when namespace, name, and version are apart of the CollectionImport
+    # for key, value in collection_upload.items():
+    #     if key in COLLECTION_METADATA.keys():
+    #         assert COLLECTION_METADATA[key] == value, collection_upload


If these do not assert anymore, i think we are seeing a change behavioural change unrelated to the bug / improvement advertised in the PR.

The asserts were never being ran in the first place. collection_upload is a CollectionImport response and does not have the fields in COLLECTION_METADATA : namespace, name, version. So commenting them out does nothing I could have left it as is.

Are you saying the keys should have been there? Then we should not guard with the "if". If not can we just remove the block completely?

No idea if they should have been there. But they will be once I add Namespaces.

pulp_ansible/tests/functional/api/collection/v3/test_collection.py

mdellweg · 2022-08-25T18:58:00Z

pulp_ansible/app/galaxy/v3/views.py

    pulp_tag_name = "Pulp_Ansible: Artifacts Collections V3"

    DEFAULT_ACCESS_POLICY = _PERMISSIVE_ACCESS_POLICY

+    _declared_fields = serializer_class._declared_fields
+    INLINE_SERIALIZER = inline_serializer(


Can we call this the FakeCollectionVersionUploadSerializer and define it outside of the class scope?

Also, can you remind me why we need to fake it?

So I can hide the Pulp-specific fields: artifact, upload, and repository from the generated schema. I tried to just not include them in the Meta class, but then it would error when artifact or repository was set by the viewset's logic.

It think, this is dangerous. Just not adding them to the docs, means that the serializer still accepts them. And it shouldn't!?

You are right. I'll add validation for this serializer.

mdellweg · 2022-08-25T19:01:13Z

pulp_ansible/app/serializers.py

+        tags = validated_data.pop("tags")
+        origin_repository = validated_data.pop("origin_repository")
+        # Create CollectionVersion from its metadata and adds to repository if specified
+        content = super().create(validated_data)


Does it make sense to add the add-to-repository-in-a-subtask feature to pulpcore?

I'm thinking about it. I already know how I am going to implement the feature in pulp_ansible, but putting it into pulpcore would make it much more simple.

mdellweg · 2022-08-25T19:15:10Z

pulp_ansible/app/tasks/upload.py

+log = logging.getLogger(__name__)
+
+
+def process_collection_artifact(artifact, namespace, name, version):


This is the thing we call in deferred_validate now? Great achievement!

Yep, extracted out the relevant parts of import_collection for gathering metadata into this.

mdellweg · 2022-08-25T19:16:36Z

pulp_ansible/app/serializers.py

+        """Check and set the namespace, name & version."""
+        # This validation is for creating CollectionVersions
+        if self.instance:
+            return data


Shouldn't we still call super first?

I'm thinking of removing this check and instead over-riding the update method to raise an error if called. validate should only be called on create or update so if self.instance is set then update will be called and we shouldn't allow it.

Edit NVM, validate gets called before update or create, so I have to leave this check here and the question is should I raise an error. If I call super it will try to call the process_collection code which is probably not what we want to do if they are trying to update. But should we allow them to use the serializer to update a collection?

Hmm. Hard question. From the API we should never allow updates. But we want to allow for a repair command. And it would be super neat if that repair command could just use this serializer to actually call the process_collection part again. No?

Yes that would be a good use. There would still need to be other fields that would need to be set if you wanted to call super in that case (artifact, expected_namespace/name/version/) else the serializer would fail validation. I'll remove the check and put a TODO comment to revisit this for a future issue (#1106).

gerrod3

@mdellweg @newswangerd Ready for review.

Summary of changes:

/ansible/collections/ & Galaxy V2 Collection upload have been deprecated.
CollectionVersionUploadSerializer now has all the old fields of CollectionOneShotSerializer and the entirety of the collection upload logic needed.
Galaxy V3 Collection upload is now a SingleArtifactContentUploadViewSet and uses the CollectionVersionUploadSerializer to perform the collection upload.
CollectionVersionSerializer now has CollectionVersionUploadSerializer as a base class so the Pulp CV api uses the same base upload logic as the Galaxy V3 api.
Pulp CV api now accepts artifact and upload fields when uploading a collection.

pulp_ansible/app/galaxy/mixins.py

pulp_ansible/app/serializers.py

gerrod3 · 2022-08-26T15:49:17Z

pulp_ansible/app/serializers.py

+    def is_valid(self, raise_exception=False):
+        """
+        Allow this serializer to be used for validating before saving a model.


I didn't touch any of the old import_collection task code, but this functionality with this serializer is how that task verifies the validity of the imported collection. Interestingly only pulp_ansible & pulp_deb follow this validation model and pulp_deb had to create two serializers to perform it.

I'm still not sure we need this. Is it there to support code we plan to deprecate/delete? If i read correctly it allows the serializer to consume validated data, right? I do not think they are designed that way.
My thinking is if we hand the same raw request data that we get in the viewset to the task, then the serializer in the task can validate in the same way plus the additional deferred_validate step. No need for any serializer field to behave differently.
Not blocking on this now. But can you confirm that this will fall for another refactor to come?

Yes it most likely will be removed once the import_collection code is finally removed. read_only fields are not validated when initialized in a serializer and they don't appear in the validate method.

gerrod3 · 2022-08-26T15:54:53Z

pulp_ansible/app/tasks/upload.py

+log = logging.getLogger(__name__)
+
+
+def process_collection_artifact(artifact, namespace, name, version):


Yep, extracted out the relevant parts of import_collection for gathering metadata into this.

gerrod3 · 2022-08-26T15:56:49Z

pulp_ansible/app/tasks/upload.py

+    # Set up logging for CollectionImport object
+    CollectionImport.objects.get_or_create(task_id=Task.current().pulp_id)
+    user_facing_logger = logging.getLogger("pulp_ansible.app.tasks.collection.import_collection")


This is set up under the assumption we are performing a collection import. A slight change would be needed here if we wanted to also use this for repairing existing collection metadata.

pulp_ansible/tests/functional/api/collection/v3/test_deletion.py

mdellweg · 2022-08-29T09:07:05Z

pulp_ansible/app/galaxy/serializers.py

+        sha256 = data["file"].hashers["sha256"].hexdigest()
+        artifact = Artifact.objects.filter(sha256=sha256).first()
+        if artifact:
+            raise serializers.ValidationError(_("Artifact already exists"))


Is it safe to just take the existing artifact at this time? (The user uploaded the proper file, right?)

No, this calls the old import_collection code and it doesn't handle duplicate artifacts so the task would fail.

Do we plan to (eventually?) change that task then too?

I eventually plan to remove the task as it is duplicate code now and eventually remove the deprecated endpoints that call it. Git sync still uses it as well so that will also require a separate PR to refactor it.

mdellweg · 2022-08-29T09:09:55Z

pulp_ansible/app/galaxy/v3/views.py

    pulp_tag_name = "Pulp_Ansible: Artifacts Collections V3"

    DEFAULT_ACCESS_POLICY = _PERMISSIVE_ACCESS_POLICY

+    _declared_fields = serializer_class._declared_fields
+    INLINE_SERIALIZER = inline_serializer(


It think, this is dangerous. Just not adding them to the docs, means that the serializer still accepts them. And it shouldn't!?

pulp_ansible/app/serializers.py

mdellweg

I'm still a bit confused by the big number of serializers we have for creating CollectionVersions. Maybe we need individual ones for each version of the api, and that is fine.
We still need confirmation from the galaxy side before we can merge this.

mdellweg · 2022-09-01T08:29:05Z

pulp_ansible/app/serializers.py

+    3. ``pulpcore.plugin.tasks.general_create`` is dispatched with this serializer, the task payload
+        and a deferred context determine by the viewset (default an empty context).
+    4. ``general_create`` calls ``validate()`` again with deferred context now.
+        ``deferred_validate()`` is now called and the upload object (if present) is converted to an


Isn't this where we also call the import_collection to extract all the metadata? That process that may be offloaded into a different application cluster?

Yes, I wrote these steps describing how the pulpcore machinery works since I feel it is not clear from just looking at the serializer what is going on.

mdellweg · 2022-09-01T08:45:01Z

pulp_ansible/app/serializers.py

+    def is_valid(self, raise_exception=False):
+        """
+        Allow this serializer to be used for validating before saving a model.


I'm still not sure we need this. Is it there to support code we plan to deprecate/delete? If i read correctly it allows the serializer to consume validated data, right? I do not think they are designed that way.
My thinking is if we hand the same raw request data that we get in the viewset to the task, then the serializer in the task can validate in the same way plus the additional deferred_validate step. No need for any serializer field to behave differently.
Not blocking on this now. But can you confirm that this will fall for another refactor to come?

[noissue] Required PR: pulp/pulp_ansible#1176

fixes: pulp#1175

No-Issue Required PR: pulp/pulp_ansible#1176

No-Issue Required PR: pulp/pulp_ansible#1176 env: LOCK_REQUIREMENTS=0 env: PULP_ANSIBLE_REVISION=0dc254376919b92ba28c4121058a844cd037d010

gerrod3 force-pushed the collection-test-fix branch from 02ee428 to d7d9b91 Compare August 17, 2022 22:10

gerrod3 marked this pull request as ready for review August 18, 2022 02:11

gerrod3 requested a review from a team as a code owner August 18, 2022 02:11

mdellweg reviewed Aug 18, 2022

View reviewed changes

pulp_ansible/app/galaxy/views.py Outdated Show resolved Hide resolved

mdellweg reviewed Aug 18, 2022

View reviewed changes

pulp_ansible/app/serializers.py Outdated Show resolved Hide resolved

gerrod3 force-pushed the collection-test-fix branch 4 times, most recently from 7176d3f to d0a2fda Compare August 18, 2022 19:38

mdellweg reviewed Aug 19, 2022

View reviewed changes

gerrod3 force-pushed the collection-test-fix branch from d0a2fda to d6d1b03 Compare August 19, 2022 20:58

gerrod3 marked this pull request as draft August 22, 2022 01:34

mdellweg reviewed Aug 22, 2022

View reviewed changes

pulp_ansible/tests/functional/api/collection/v3/test_collection.py Show resolved Hide resolved

gerrod3 force-pushed the collection-test-fix branch 2 times, most recently from 14c1616 to 4788bc8 Compare August 24, 2022 15:42

gerrod3 changed the title ~~Fix duplicate collection upload error msg & tests~~ Refactor CollectionVersion Upload to use pulpcore machinery Aug 24, 2022

gerrod3 force-pushed the collection-test-fix branch 3 times, most recently from fe2c1c7 to 8538fed Compare August 25, 2022 18:14

mdellweg reviewed Aug 25, 2022

View reviewed changes

gerrod3 force-pushed the collection-test-fix branch 5 times, most recently from c42344c to 11b02ad Compare August 26, 2022 04:40

gerrod3 commented Aug 26, 2022

View reviewed changes

gerrod3 marked this pull request as ready for review August 26, 2022 16:26

mdellweg reviewed Aug 29, 2022

View reviewed changes

gerrod3 force-pushed the collection-test-fix branch 2 times, most recently from d148e42 to 00e8775 Compare August 29, 2022 18:26

gerrod3 force-pushed the collection-test-fix branch from 00e8775 to a48903d Compare August 29, 2022 18:48

gerrod3 requested a review from mdellweg August 31, 2022 01:27

gerrod3 force-pushed the collection-test-fix branch from a48903d to d172762 Compare September 1, 2022 01:30

mdellweg approved these changes Sep 1, 2022

View reviewed changes

mdellweg marked this pull request as draft September 12, 2022 13:41

gerrod3 force-pushed the collection-test-fix branch from d172762 to c21565b Compare September 26, 2022 00:45

gerrod3 added a commit to gerrod3/galaxy_ng that referenced this pull request Sep 26, 2022

Use pulp_ansible collection upload refactor

533bcaa

[noissue] Required PR: pulp/pulp_ansible#1176

gerrod3 mentioned this pull request Sep 26, 2022

Use pulp_ansible collection upload refactor ansible/galaxy_ng#1463

Merged

newswangerd removed the request for review from a team October 4, 2022 16:23

gerrod3 force-pushed the collection-test-fix branch 2 times, most recently from b95db90 to a0dd173 Compare November 17, 2022 05:13

gerrod3 added a commit to gerrod3/galaxy_ng that referenced this pull request Nov 17, 2022

Use pulp_ansible collection upload refactor

4aef651

[noissue] Required PR: pulp/pulp_ansible#1176

gerrod3 force-pushed the collection-test-fix branch 2 times, most recently from c479463 to 0b03aa0 Compare November 17, 2022 16:18

Refactor CollectionVersion Upload to use pulpcore machinery

0dc2543

fixes: pulp#1175

gerrod3 force-pushed the collection-test-fix branch from 0b03aa0 to 0dc2543 Compare November 17, 2022 17:01

gerrod3 added a commit to gerrod3/galaxy_ng that referenced this pull request Nov 21, 2022

Use pulp_ansible collection upload refactor

eefb531

No-Issue Required PR: pulp/pulp_ansible#1176

gerrod3 added a commit to gerrod3/galaxy_ng that referenced this pull request Nov 21, 2022

Use pulp_ansible collection upload refactor

47aeb58

No-Issue Required PR: pulp/pulp_ansible#1176

gerrod3 added a commit to gerrod3/galaxy_ng that referenced this pull request Nov 21, 2022

Use pulp_ansible collection upload refactor

ac51d20

No-Issue Required PR: pulp/pulp_ansible#1176 env: LOCK_REQUIREMENTS=0 env: PULP_ANSIBLE_REVISION=0dc254376919b92ba28c4121058a844cd037d010

gerrod3 marked this pull request as ready for review November 30, 2022 20:53

mdellweg approved these changes Dec 1, 2022

View reviewed changes

gerrod3 merged commit 357cdca into pulp:main Dec 1, 2022

gerrod3 deleted the collection-test-fix branch December 1, 2022 16:07

gerrod3 mentioned this pull request Jan 23, 2023

Ansible collection upload URL missing /pulp/api/v3/ #686

Closed

gerrod3 mentioned this pull request Dec 6, 2023

CLI does not send the file name in the form data on file uploads. pulp/pulp-cli#842

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor CollectionVersion Upload to use pulpcore machinery #1176

Refactor CollectionVersion Upload to use pulpcore machinery #1176

gerrod3 commented Aug 17, 2022

mdellweg Aug 19, 2022

mdellweg Aug 19, 2022

gerrod3 Aug 19, 2022

mdellweg Aug 19, 2022

gerrod3 Aug 19, 2022

mdellweg Aug 22, 2022

gerrod3 Aug 24, 2022

mdellweg Aug 25, 2022

gerrod3 Aug 25, 2022

mdellweg Aug 29, 2022

gerrod3 Aug 29, 2022

mdellweg Aug 25, 2022

gerrod3 Aug 25, 2022

mdellweg Aug 25, 2022

gerrod3 Aug 26, 2022

mdellweg Aug 25, 2022

gerrod3 Aug 25, 2022 •

edited

mdellweg Aug 26, 2022

gerrod3 Aug 26, 2022

gerrod3 left a comment

gerrod3 Aug 26, 2022

mdellweg Sep 1, 2022

gerrod3 Sep 1, 2022

gerrod3 Aug 26, 2022

gerrod3 Aug 26, 2022

mdellweg Aug 29, 2022

gerrod3 Aug 29, 2022

mdellweg Aug 30, 2022

gerrod3 Aug 30, 2022

mdellweg Aug 29, 2022

mdellweg left a comment

mdellweg Sep 1, 2022

gerrod3 Sep 1, 2022

mdellweg Sep 1, 2022

		log = logging.getLogger(__name__)


		def process_collection_artifact(artifact, namespace, name, version):

Refactor CollectionVersion Upload to use pulpcore machinery #1176

Refactor CollectionVersion Upload to use pulpcore machinery #1176

Conversation

gerrod3 commented Aug 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerrod3 Aug 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerrod3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdellweg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerrod3 Aug 25, 2022 •

edited