Add Repository.finalize_new_version #369

bmbouter · 2019-11-07T15:25:42Z

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 #3541

pulpcore/plugin/stages/declarative_version.py

goosemania

Thank you!

new changes are on the way

… and sync. Also removes one_shot_upload as it's not used anymore. Required PR: pulp/pulpcore#369 https://pulp.plan.io/issues/3541 re #3541

Required PR: pulp/pulpcore#369 closes #3541

bmbouter · 2019-11-12T19:33:21Z

@gmbnomis and @mdellweg I talked w/ @dralley, @daviddavis, @dkliban, and @goosemania; here is what we came up with:

We don't have a use case where the user wants to provide configuration of the vadifier currently. Even if we end up with one the plugin writer could still work around it because they are the owner of the viewset now.
Therefore moving the vadifier to a central place that SingleContentArtifactUploadSerializer.create, DeclarativeVersion.run, and modify can call is good to avoid passing it through the tasking system and into the core machinery with optional params.
We observe that it would be useful for the vodifier to have access to the Repository instance. For example a "retain N" feature for pulp_rpm. The user won't want to specify that call-by-call, but instead set it on a Repository itself as, e.g. retain=2, which now they can do with typed repos. Then having the vadifier have access to that Repository instance would be good. Thus making it an instance method is useful because it has access to self.
I got feedback that the vadifier name was confusing. We're planning to name this def new_version_finalize(self, new_version).
This would be useful for various other things also so it creates more value for the plugin writer, e.g. notifying a third party service upon finalization maybe?

I'm going to add this in as an additional commit to demonstrate the idea with the hopes of merging it. What do you think of this? Will this work your your needs?

daviddavis · 2019-11-12T19:39:02Z

Does finalize_new_version make sense over new_version_finalize?

bmbouter · 2019-11-12T19:49:34Z

Yes I'll use finalize_new_version instead.

mdellweg · 2019-11-12T19:51:23Z

If i read correctly, the suggestion now is to make the validitator an instance method of repository, and any possible parameter to it a field on said repository. This is exactly, what i thought it to be, as i see this method to put some common (for all operations) constraints on the resulting repository versions.
And for the example of recursively adding dependencies, i would go as far as stating, that if one wants that (repo cosure) on a specific repository, than that validification should also be used on sync.
We would even need to think about the possibility, that changing such a flag on the repository might create a new repoversion to satisfy it.

mdellweg · 2019-11-12T19:59:10Z

Because vadifiers also include modification, I think they don't belong to repo. They belong to the respective operation creating a repo version. The vadifiers are there to allow to reuse common parts (which may be 100% in many cases).

And i would argue here, that they belong to the repository to ensure the same constraints for all (future) versions independently of the operation performed.

Required PR: pulp/pulpcore#369 closes #3541

dralley · 2019-11-12T21:55:45Z

pulpcore/plugin/repo_version_utils.py

+    """
+    query_for_repo_duplicates_by_type = defaultdict(lambda: Q())
+    for item in repository_version.added():
+        detail_item = item.cast()


Future performance improvement to take note of

Sort these into groups based on their pulp_type which is present on the master Content model.

Look up the detail content models that represent the pulp_type strings

Query the detail content models directly, in bulk, provided a list of PKs, instead of cast() individually

Then within each type group check for duplicates

Or, alternately, each repository can list all of the content types it supports and step 2 can be skipped, and we can add an extra layer of protection around making sure you can't have e.g. file content in an RPM repository which we can't easily or centrally guarantee otherwise.

Thanks for filing @dralley https://pulp.plan.io/issues/5701

mdellweg · 2019-11-13T06:58:01Z

pulpcore/plugin/actions.py

+        if 'remove_content_units' in request.data:
+            for url in request.data['remove_content_units']:
+                if url == '*':
+                    remove_content_units.append(url)


Does * mean remove everything?
Should we do

if url == "*": remove_content_units=["*"] break

Yes I agree with the break change because when at least one remove_content_units specifies to remove all, additional remove_content_units params specified are unproductive.

I am going to leave remove_content_units=[url] because it DRYs the definition of *.

pulpcore/app/models/content.py

mdellweg · 2019-11-13T07:05:58Z

pulpcore/plugin/actions.py

+        add_content_units = []
+        remove_content_units = []
+        repository = Repository.objects.get(pk=pk)
+        repository.cast()


Is this detail_repository = repository.cast() ?

Yes this line is unproductive since the result is never saved. It's also not needed since we only use the pk and that's the same for both master and detail. Also I verified that when the line is removed a repository PATCH and a later 'modify' call both use the same reserved_resource so they will be serialized correctly. I'm pushing the change that removes this line now.

Also removes one_shot_upload as it's not used anymore. Required PR: pulp/pulpcore#369 closes #4898 https://pulp.plan.io/issues/4898

Required PR: pulp/pulpcore#369 closes #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

goosemania · 2019-11-13T18:45:00Z

CHANGES/plugin_api/3541.removal

@@ -0,0 +1,4 @@
+Renamed the Content.repo_key to be Content.repo_unit_key. Also the calling of `RemoveDuplicates`


Did you mean Content.repo_key_fields?

And also RemoveDuplicates is now remove_duplicates

yes but I want to fix in separate PR because I need to unblock the other Prs with the skiptest in this one. Is that ok?

followup PR here: #382

goosemania · 2019-11-13T18:48:25Z

pulpcore/plugin/repo_version_utils.py

+    a `repo_key_fields` attribute with the field names to be compared. If all `repo_key_fields`
+    contain the same value for two content units, they are considered "repository duplicates".
+
+    After instantiating `RemoveDuplicates` call it with the `run()` method and pass in the


remove_duplicates

And there is no longer run since it's just a function

Followup PR here: #382

goosemania

Thanks!

Just a couple of docs comments which can be addressed in a follow-up PR

bmbouter · 2019-11-13T18:52:02Z

Thank you! I will make the followup PR now and ensure it's merged pre-branch and pre-RC8.

Adapt to pulp/pulpcore#369 (Repository /modify mixin) and pulp/pulpcore#380 (Don't create a new repository version if no content was added or removed). No functional changes.

bmbouter added the Work In Progress label Nov 7, 2019

bmbouter force-pushed the 3541 branch 2 times, most recently from f73c932 to a985533 Compare November 7, 2019 21:18

bmbouter changed the title ~~DeclarativeVersion no longer creates RepoVersion~~ Move RepoVersion create to plugins Nov 7, 2019

bmbouter mentioned this pull request Nov 7, 2019

Sync, Upload, Modify preserve uniqueness pulp/pulp_file#307

Merged

bmbouter removed the Work In Progress label Nov 7, 2019

bmbouter force-pushed the 3541 branch 3 times, most recently from e4c6b80 to b90bd64 Compare November 7, 2019 22:02

bmbouter requested a review from a team November 7, 2019 22:02

dralley reviewed Nov 10, 2019

View reviewed changes

pulpcore/plugin/stages/declarative_version.py Outdated Show resolved Hide resolved

dralley reviewed Nov 10, 2019

View reviewed changes

pulpcore/plugin/stages/declarative_version.py Outdated Show resolved Hide resolved

bmbouter force-pushed the 3541 branch from b90bd64 to f88f788 Compare November 11, 2019 20:03

goosemania reviewed Nov 11, 2019

View reviewed changes

pulpcore/plugin/stages/declarative_version.py Outdated Show resolved Hide resolved

goosemania previously approved these changes Nov 11, 2019

View reviewed changes

bmbouter force-pushed the 3541 branch 3 times, most recently from 3ae4c55 to ea94c1f Compare November 11, 2019 21:46

bmbouter force-pushed the 3541 branch from ea94c1f to e96c3a0 Compare November 11, 2019 22:22

goosemania mentioned this pull request Nov 11, 2019

Ensure no duplicates are present in a repo version pulp/pulp_rpm#1503

Merged

ipanova added a commit to ipanova/pulp_container that referenced this pull request Nov 12, 2019

Use RemoveDuplicates utility when creating repo version.

e7e65bd

Required PR: pulp/pulpcore#369 closes #3541

ipanova mentioned this pull request Nov 12, 2019

No duplicated content can be present in a repository version. pulp/pulp_container#3

Merged

ipanova added a commit to ipanova/pulp_container that referenced this pull request Nov 12, 2019

Use RemoveDuplicates utility when creating repo version.

fac94cd

Required PR: pulp/pulpcore#369 closes #3541

ipanova added a commit to ipanova/pulp_container that referenced this pull request Nov 12, 2019

Use RemoveDuplicates utility when creating repo version.

b45720c

Required PR: pulp/pulpcore#369 closes #3541

bmbouter force-pushed the 3541 branch from 67aad0a to 8bc8376 Compare November 12, 2019 14:18

ipanova added a commit to ipanova/pulp_container that referenced this pull request Nov 12, 2019

Use RemoveDuplicates utility when creating repo version.

e089e6a

Required PR: pulp/pulpcore#369 closes #3541

bmbouter force-pushed the 3541 branch 2 times, most recently from 49540c9 to e222024 Compare November 12, 2019 21:23

bmbouter changed the title ~~Move RepoVersion create to plugins~~ Add Repository.finalize_new_version Nov 12, 2019

dralley reviewed Nov 12, 2019

View reviewed changes

mdellweg reviewed Nov 13, 2019

View reviewed changes

pulpcore/app/models/content.py Outdated Show resolved Hide resolved

mdellweg reviewed Nov 13, 2019

View reviewed changes

goosemania added a commit to goosemania/pulp_rpm that referenced this pull request Nov 13, 2019

Ensure no duplicates are present in a repo version.

1922cd9

Also removes one_shot_upload as it's not used anymore. Required PR: pulp/pulpcore#369 closes #4898 https://pulp.plan.io/issues/4898

goosemania added a commit to goosemania/pulp_rpm that referenced this pull request Nov 13, 2019

Ensure no duplicates are present in a repo version.

7158604

Also removes one_shot_upload as it's not used anymore. Required PR: pulp/pulpcore#369 closes #4898 https://pulp.plan.io/issues/4898

ipanova added a commit to ipanova/pulp_container that referenced this pull request Nov 13, 2019

No duplicated content can be present in a repository version.

7812b08

Required PR: pulp/pulpcore#369 closes #3541

ipanova added a commit to ipanova/pulp_container that referenced this pull request Nov 13, 2019

No duplicated content can be present in a repository version.

16ec458

Required PR: pulp/pulpcore#369 closes #3541

bmbouter force-pushed the 3541 branch 2 times, most recently from b2e39b5 to d681090 Compare November 13, 2019 16:27

bmbouter force-pushed the 3541 branch from d681090 to 09fe658 Compare November 13, 2019 18:14

goosemania reviewed Nov 13, 2019

View reviewed changes

goosemania approved these changes Nov 13, 2019

View reviewed changes

bmbouter merged commit 93530a9 into pulp:master Nov 13, 2019

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

gmbnomis mentioned this pull request Nov 18, 2019

Adapt to remaining pulpcore 3.0.0rc8 changes pulp/pulp_cookbook#67

Merged

Add Repository.finalize_new_version #369

Add Repository.finalize_new_version #369

bmbouter commented Nov 7, 2019 •

edited

goosemania left a comment

bmbouter commented Nov 12, 2019

daviddavis commented Nov 12, 2019

bmbouter commented Nov 12, 2019

mdellweg commented Nov 12, 2019

mdellweg commented Nov 12, 2019

dralley Nov 12, 2019 •

edited

bmbouter Nov 13, 2019

mdellweg Nov 13, 2019

bmbouter Nov 13, 2019

mdellweg Nov 13, 2019

bmbouter Nov 13, 2019

goosemania Nov 13, 2019

goosemania Nov 13, 2019

bmbouter Nov 13, 2019

goosemania Nov 13, 2019

bmbouter Nov 13, 2019

goosemania Nov 13, 2019 •

edited

bmbouter Nov 13, 2019

goosemania left a comment

bmbouter commented Nov 13, 2019

		@@ -0,0 +1,4 @@
		Renamed the Content.repo_key to be Content.repo_unit_key. Also the calling of `RemoveDuplicates`

Add Repository.finalize_new_version #369

Add Repository.finalize_new_version #369

Conversation

bmbouter commented Nov 7, 2019 • edited

goosemania left a comment

Choose a reason for hiding this comment

bmbouter commented Nov 12, 2019

daviddavis commented Nov 12, 2019

bmbouter commented Nov 12, 2019

mdellweg commented Nov 12, 2019

mdellweg commented Nov 12, 2019

dralley Nov 12, 2019 • edited

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

goosemania Nov 13, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goosemania left a comment

Choose a reason for hiding this comment

bmbouter commented Nov 13, 2019

bmbouter commented Nov 7, 2019 •

edited

dralley Nov 12, 2019 •

edited

goosemania Nov 13, 2019 •

edited