Sync, Upload, Modify preserve uniqueness #307

bmbouter · 2019-11-07T21:11:13Z

Sync, Upload, Modify preserve uniqueness

Sync, Upload, and Modify now have added content with the same
relative_path as existing content will remove the existing content.

Required PR: pulp/pulpcore#369

https://pulp.plan.io/issues/3541
re #3541

This change has several parts: DeclarativeVersion no longer creates RepoVersion As part of allowing plugin validation, the `DeclarativeVersion` object no longer: 1) creates the RepositoryVersion 2) finalizes the RepositoryVersion The initialization of DeclarativeVersion has chaged replacing the `repository` parameter with the `repository_version` parameter requiring the plugin writer to create it. Also Remove the repo_key implementation from add_content, and move it a utility function named RemoveDuplicates so plugin writers can call it. This also renamed `repo_key` to `repo_key_fields`. Required PR: pulp/pulp_file#307 https://pulp.plan.io/issues/3541 re pulp#3541

dralley · 2019-11-07T21:31:53Z

pulp_file/app/tasks/synchronizing.py

+    with RepositoryVersion.create(repository) as new_version:
+        dv = DeclarativeVersion(first_stage, new_version, mirror=mirror)
+        dv.create()
+        RemoveDuplicates(new_version).run()


I really like this. I bet it makes custom pipelines a lot easier too.

This change has several parts: DeclarativeVersion no longer creates RepoVersion As part of allowing plugin validation, the `DeclarativeVersion` object no longer: 1) creates the RepositoryVersion 2) finalizes the RepositoryVersion The initialization of DeclarativeVersion has chaged replacing the `repository` parameter with the `repository_version` parameter requiring the plugin writer to create it. Also Remove the repo_key implementation from add_content, and move it a utility function named RemoveDuplicates so plugin writers can call it. This also renamed `repo_key` to `repo_key_fields`. Required PR: pulp/pulp_file#307 https://pulp.plan.io/issues/3541 re pulp#3541

gmbnomis · 2019-11-10T11:26:35Z

pulp_file/app/tasks/synchronizing.py

+    with RepositoryVersion.create(repository) as new_version:
+        dv = DeclarativeVersion(first_stage, new_version, mirror=mirror)
+        dv.create()
+        RemoveDuplicates(new_version).run()


I would have expected a pattern like RemoveDuplicates().run(new_version) . If we were to pass around validators/modifiers in the future, this would allow to prepare validator instances (with config options) and use them in generic create/add_remove tasks. wdyt?

+1 to this I'm going to make this change. It seems more flexible for the caller to have the reference to new_version for the generic create/add_remove tasks as you mention.

This change has several parts: DeclarativeVersion no longer creates RepoVersion As part of allowing plugin validation, the `DeclarativeVersion` object no longer: 1) creates the RepositoryVersion 2) finalizes the RepositoryVersion The initialization of DeclarativeVersion has chaged replacing the `repository` parameter with the `repository_version` parameter requiring the plugin writer to create it. Also Remove the repo_key implementation from add_content, and move it a utility function named RemoveDuplicates so plugin writers can call it. This also renamed `repo_key` to `repo_key_fields`. Required PR: pulp/pulp_file#307 https://pulp.plan.io/issues/3541 re pulp#3541

bmbouter · 2019-11-11T20:48:11Z

pulp_file/app/serializers.py

+            # create new repo version with uploaded package
+            with repository.new_version() as new_version:
+                new_version.add_content(content_to_add)
+                RemoveDuplicates().run(new_version)


With the new_version passed in as part of run(new_version), this could work differently. We could introduce an optional param on SingleArtifactContentUploadSerializer.create in core called plugin_modify_validate which the plugin could pass in.

That would make this function instead:

def create(self, validated_data): return super().create(validated_data, plugin_modify_validate=RemoveDuplicates())

@bmbouter This would be an improvement over what we currently have. Let's do it.

@gmbnomis this was made possible by your suggestion to have the called of run() provide new_version. What do you think of me making ^ change?

@bmbouter, how will you call the plugin_modify_validate in core? plugin_modify_validate.run(new_version)?
I'm just trying to understand the interface if I have multiple modifiers/validators in plugin. I guess I can combine all of them in one (if they can run after content was added).

Yes that's how I imagined it would be called. It's only prepared to call a single item so the plugin writer would wrap multiple ones together. Another option would be to have a list or optionall have plugin_modify_validate accept either a single item or a list of items to call in order.

I think it's fine to call one combined one and if plugin needs to do something in between, it has flexibility to do so.

+1 to your proposal

@bmbouter although it is "hooky", I like the pattern. The hook has a well defined boundary which is rather independent of how create is actually implemented. (the previous solution could break easily with changes to create in pulpcore).

However, create() now takes a different number of arguments depending on which class it is defined in, which isn't very nice. Should we add an instance member that stores the validator/modifier instance (with a class member that defaults to None)? Then, one would simply set it e.g. in the subclass (class member), in an overloaded init or from the outside before calling create (i.e. the pattern that you see all the time in Django/DRF).

@gmbnomis that is a great idea, let me put that together. Do you have a suggestion for the class member name?

This change has several parts: DeclarativeVersion no longer creates RepoVersion As part of allowing plugin validation, the `DeclarativeVersion` object no longer: 1) creates the RepositoryVersion 2) finalizes the RepositoryVersion The initialization of DeclarativeVersion has chaged replacing the `repository` parameter with the `repository_version` parameter requiring the plugin writer to create it. Also Remove the repo_key implementation from add_content, and move it a utility function named RemoveDuplicates so plugin writers can call it. This replaces the RemoveDuplicates stage which has been fully removed. Additionally, the `DeclarativeVersion` no longer accepts `remove_duplicates` as an argument. Plugin writers are expected to use plugin modify/validate facilities to remove duplicates now. This also renamed `repo_key` to `repo_key_fields`. Required PR: pulp/pulp_file#307 https://pulp.plan.io/issues/3541 re pulp#3541

Required PR: pulp/pulp_file#307 https://pulp.plan.io/issues/3541 re pulp#3541

This change has several parts: DeclarativeVersion no longer creates RepoVersion As part of allowing plugin validation, the `DeclarativeVersion` object no longer: 1) creates the RepositoryVersion 2) finalizes the RepositoryVersion The initialization of DeclarativeVersion has chaged replacing the `repository` parameter with the `repository_version` parameter requiring the plugin writer to create it. Also Remove the repo_key implementation from add_content, and move it a utility function named RemoveDuplicates so plugin writers can call it. This replaces the RemoveDuplicates stage which has been fully removed. Additionally, the `DeclarativeVersion` no longer accepts `remove_duplicates` as an argument. Plugin writers are expected to use plugin modify/validate facilities to remove duplicates now. This also renamed `repo_key` to `repo_key_fields`. Required PR: pulp/pulp_file#307 https://pulp.plan.io/issues/3541 re pulp#3541

Required PR: pulp/pulp_file#307 https://pulp.plan.io/issues/3541 re pulp#3541

mdellweg · 2019-11-12T15:48:40Z

pulp_file/app/tasks/synchronizing.py

@@ -41,7 +42,7 @@ def synchronize(remote_pk, repository_pk, mirror):
        raise ValueError(_("A remote must have a url specified to synchronize."))

    first_stage = FileFirstStage(remote)
-    dv = DeclarativeVersion(first_stage, repository, mirror=mirror)
+    dv = DeclarativeVersion(first_stage, repository, mirror=mirror, vadifier=RemoveDuplicates().run)


Did you mean validifier or validator?

I meant both because it could perform either. The feedback is that folks want a different name so we're going to rename it once we determine what a better name is.

mdellweg · 2019-11-12T16:02:06Z

pulp_file/app/viewsets.py

-class FileRepositoryViewSet(RepositoryViewSet):
+class FileVadifiyModifyActionMixin(ModifyActionMixin):
+
+    def _dispatch_task(self, *args, **kwargs):


This _dispatch_task is not called on sync, right?
What tasks is it called on? I believe there is not much use on delete.
Also should we really pass inflated objects to the task queue?

This _dispatch_task is not called on sync, right?

Correct, it's called as part of the modify endpoint. With typed repos merged, this endpoint was provided on all Repository viewsets, but now it's an opt-in endpoint with plugin writers adding the mixin to their Detail RepositoryViewset, e.g. L#68.

What tasks is it called on? I believe there is not much use on delete.

This is in the viewset, but it's going to call the add_and_remove task

Also should we really pass inflated objects to the task queue?

This is also a concern of mine, but I'm not totally sure why other than pickling non-native Python types makes me nervous. RQ pickles its arguments so I'm not concerned with it working. I am a little concerned though.

Required PR: pulp/pulp_file#307 https://pulp.plan.io/issues/3541 re pulp#3541

Sync, Upload, and Modify now have added content with the same `relative_path` as existing content will remove the existing content. Required PR: pulp/pulpcore#369 https://pulp.plan.io/issues/3541 re #3541

This PR adds a central place for plugin writers to put validation and content modification code. * `RepositoryVersion.__exit__` calls `finalize_new_version` * Moves the `/modify/` endpoint to `pulpcore.plugin.actions` as a mixin named `ModifyRepositoryActionMixin`. * Renames `Repository.repo_key` to `Repository.repo_key_fields`. * Remove the `RemoveDuplicates` stage for plugin writers to instead use `Repository.finalize_new_version` * Remove the implicit usage of `RemoveDuplicates` from `RepositoryVerison.add_content` and `RepositoryVersion.remove_content`. * Make the `RemoveDuplicates` available as `pulpcore.plugin.repo_version_utils.remove_duplicates`. As an unrelated change, it also replaces the the `>>>` python codeblocks in various docblocks with python codeblock using `::` and indention. Required PR: pulp/pulp_file#307 https://pulp.plan.io/issues/3541 re pulp#3541

bmbouter mentioned this pull request Nov 7, 2019

Add Repository.finalize_new_version pulp/pulpcore#369

Merged

dralley reviewed Nov 7, 2019

View reviewed changes

gmbnomis reviewed Nov 10, 2019

View reviewed changes

bmbouter commented Nov 11, 2019

View reviewed changes

bmbouter added a commit to bmbouter/pulpcore that referenced this pull request Nov 11, 2019

Introducing vadifier!

67aad0a

Required PR: pulp/pulp_file#307 https://pulp.plan.io/issues/3541 re pulp#3541

bmbouter added a commit to bmbouter/pulpcore that referenced this pull request Nov 12, 2019

Introducing vadifier!

8bc8376

Required PR: pulp/pulp_file#307 https://pulp.plan.io/issues/3541 re pulp#3541

bmbouter added a commit to bmbouter/pulpcore that referenced this pull request Nov 12, 2019

Introducing vadifier!

f396343

Required PR: pulp/pulp_file#307 https://pulp.plan.io/issues/3541 re pulp#3541

bmbouter added a commit to bmbouter/pulpcore that referenced this pull request Nov 12, 2019

Move modify to pulpcore.plugin.actions

651a0b7

Required PR: pulp/pulp_file#307 https://pulp.plan.io/issues/3541 re pulp#3541

mdellweg reviewed Nov 12, 2019

View reviewed changes

bmbouter added a commit to bmbouter/pulpcore that referenced this pull request Nov 12, 2019

RepositoryVersion calls finalize_new_version

4067097

Required PR: pulp/pulp_file#307 https://pulp.plan.io/issues/3541 re pulp#3541

bmbouter added a commit to bmbouter/pulpcore that referenced this pull request Nov 12, 2019

RepositoryVersion calls finalize_new_version

49540c9

Required PR: pulp/pulp_file#307 https://pulp.plan.io/issues/3541 re pulp#3541

Sync, Upload, Modify preserve uniqueness

2f92991

Sync, Upload, and Modify now have added content with the same `relative_path` as existing content will remove the existing content. Required PR: pulp/pulpcore#369 https://pulp.plan.io/issues/3541 re #3541

bmbouter changed the title ~~Upload and Sync now create RepoVersion + validate~~ Sync, Upload, Modify preserve uniqueness Nov 12, 2019

dralley approved these changes Nov 12, 2019

View reviewed changes

bmbouter merged commit 86a0f85 into pulp:master Nov 13, 2019

bmbouter deleted the 3541 branch November 13, 2019 18:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync, Upload, Modify preserve uniqueness #307

Sync, Upload, Modify preserve uniqueness #307

bmbouter commented Nov 7, 2019 •

edited

Loading

dralley Nov 7, 2019

gmbnomis Nov 10, 2019

bmbouter Nov 11, 2019

bmbouter Nov 11, 2019 •

edited

Loading

dkliban Nov 11, 2019

bmbouter Nov 11, 2019

goosemania Nov 11, 2019

bmbouter Nov 11, 2019

goosemania Nov 11, 2019

gmbnomis Nov 11, 2019

bmbouter Nov 11, 2019

mdellweg Nov 12, 2019

bmbouter Nov 12, 2019

mdellweg Nov 12, 2019

bmbouter Nov 12, 2019 •

edited

Loading

Sync, Upload, Modify preserve uniqueness #307

Sync, Upload, Modify preserve uniqueness #307

Conversation

bmbouter commented Nov 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmbouter Nov 11, 2019 • edited Loading

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

bmbouter Nov 12, 2019 • edited Loading

Choose a reason for hiding this comment

bmbouter commented Nov 7, 2019 •

edited

Loading

bmbouter Nov 11, 2019 •

edited

Loading

bmbouter Nov 12, 2019 •

edited

Loading