Add content copy feature #1328

dralley · 2019-04-20T04:53:17Z

Also moved upload under app/tasks, because it's a task

closes #4716
https://pulp.plan.io/issues/4716

dralley · 2019-04-20T05:03:41Z

Parameter names are temporary, we can vote on what to do about those.

Things to discuss (preferably on the issue: https://pulp.plan.io/issues/4716)

Should we be taking a repository version as a source, instead of a repository? Or allow it to be specified as either a repository version OR a repository, like we do with publications?
Do we need to allow changing the "base_version" to use when creating the new repository version in the destination repo, also?

dralley · 2019-04-22T15:28:38Z

pulp_rpm/app/tasks/copy.py

+        source_repo: repository to copy units from
+        dest_repo: repository to copy units to
+    """
+    content_to_copy = RepositoryVersion.latest(source_repo).content


If we're leaving this in the plugin, I would suggest that we limit it to only copying RPMs and Errata, and not everything in .content

@dralley that makes sense to me. Let's also clearly label the feature as only handling those types.

bmbouter · 2019-04-22T15:33:30Z

pulp_rpm/app/viewsets.py

+    Args:
+        source_repo: repository to copy units from
+        dest_repo: repository to copy units to
+    Optional:


This line looks out of place.

bmbouter

All this code looks great! I want to request documentation for the feature that we can merge along with the feature. I think maybe a workflow section? Anyways, somewhere in the docs.

nixocio · 2019-04-22T15:51:03Z

@dralley. It would be nice if we could add documentation, mainly the work flows for an user point view, around new features as we add them.

nixocio · 2019-04-22T16:48:34Z

@dralley, is this a simple copy? No dependencies resolution, right?

dralley · 2019-04-22T16:51:47Z

@kersommoura correct

pep8speaks · 2019-04-25T20:30:07Z

Hello @dralley! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-05-23 18:54:07 UTC

ipanova · 2019-04-26T11:31:57Z

docs/workflows/copy.rst

+Copy RPM content between repositories
+=====================================
+
+If you want to copy all of the RPM content in one repository into another repository, you can do so.


s/in one/from a repository?

ipanova · 2019-04-26T11:37:58Z

docs/workflows/copy.rst

+
+You can also specify which types of content you would like to copy by providing a value for the "types" parameter. Types that are not listed will not be copied. The supported types are "packages" and "errata". For example, this query will copy only errata, and not packages.
+
+``http POST http://localhost:24817/pulp/api/v3/rpm/copy/ source_repo=${SRC_REPO_HREF} dest_repo=${DEST_REPO_HREF} types:="[\"errata\"]"``


do make the connection of errata and update record terminology in our docs? users copy errata and in the repo version summary they see instead update record. I am wondering - won't it be better if we just stick to update record?
this reminds me of our bitter experience in pulp2 about tasks state naming failed vs errored, sucessful vs finished.

nevermind, the endpoint for errata is /pulp/api/v3/content/rpm/errata/ so i guess it's ok

@dralley any reason we did not go with comma separated values? types=rpm,errata

@ipanova only because this is the way things were already being done for
"add_content_units="

@ipanova Personally I think we should also change the _type string so that they show up as errata in the repository version also. The only place I've ever seen the update record terminology used is in the createrepo_c source code - I can't seem to find any other documentation calling it that vs. errata. So I don't think it's a secret "official" name vs. a marketing term or anything like that.

ipanova · 2019-04-26T12:19:12Z

pulp_rpm/app/constants.py

+)
+
+RPM_PLUGIN_TYPE_CHOICE_MAP = {
+    'packages': RPM_PLUGIN_TYPES.PACKAGE,


there is a typo here which leads to

{ "non_field_errors": [ "'package' is an invalid type, please use one of ['packages', 'errata']" ] }

s/packages/package

ipanova · 2019-04-26T12:25:49Z

@dralley i tested copy and works ok, i also tested invalid content types and copy of rpm related content from a mixed repo that contained rpm+ docker stuff

nixocio · 2019-04-26T12:38:49Z

I will write tests for this feature as well.

dralley · 2019-04-26T13:47:25Z

pulp_rpm/app/serializers.py

+        default=['package', 'errata']
+    )
+
+    def validate(self, data):


I'm not thrilled with this implementation - anyone know a better way?

goosemania · 2019-04-26T13:57:05Z

pulp_rpm/app/constants.py

+
+RPM_PLUGIN_TYPE_CHOICE_MAP = {
+    'packages': RPM_PLUGIN_TYPES.PACKAGE,
+    'errata': RPM_PLUGIN_TYPES.ERRATA


Errata is a plural of Erratum, so if we use singular form for packages, we should use singular form for errata as well.

goosemania · 2019-04-26T14:03:57Z

pulp_rpm/app/constants.py

@@ -1,5 +1,16 @@
 from types import SimpleNamespace

+
+RPM_PLUGIN_TYPES = SimpleNamespace(
+    PACKAGE='rpm.package',


This is the type of hardcode I hoped to avoid. Is it bad to do:
PACKAGE = models.Package.TYPE ? Is it because of prepending with app_label that we need to hardcode the type?

Is it because of prepending with app_label that we need to hardcode the type?

Yup, because the value saved in _type is not the same as models.Package.TYPE

Likely not a popular opinion:
Maybe we can use existing names and if we don't go fancy with simple namespaces, it can be done [arguably] easier.
E.g. for the available types use Package.TYPE,
for the type to look for in db add app_label to it.

content_filter_choice = (Package, UpdateRecord) supported_types = [model.TYPE for model in content_filter_choice] # to validate users' input final_types = ['{app_label}.{type}'.format(app_label=model._meta.app_label, type=model.TYPE) for model in user_specified_types]

WDYT?

We can do this as part of the refactor if you want, I'm not opposed. If others are fine with it also than it works for me.

We can do this as part of the refactor if you want, I'm not opposed. If others are fine with it also than it works for me. I'm not sure I want to do it right now though, I'd rather just get it merged.

goosemania · 2019-04-26T14:12:59Z

pulp_rpm/app/serializers.py

+        source_repo = data.get('source_repo')
+        source_repo_version = data.get('source_repo_version')
+
+        if not source_repo and not source_repo_version:


An alternative way as an attempt to improve readability (not critical and maybe not a better option):
How about to eliminate bad combinarions first and then deal with the rest one by one?

if repo and repo_version: raise ... if not repo and not repo_version: raise ... if repo: ... if repo_version: ...

goosemania · 2019-04-26T15:41:53Z

pulp_rpm/app/serializers.py

+                  "but not both.")
+            )
+
+        if not source_repo and source_repo_version:


nitpicking: you already know that only one of them is set, so if source_repo_version: is enough.
Same for the source_repo condition below.

I'd prefer to leave the logic there just to make what it's doing more clear, even if it's not strictly necessary

Sure, that's subjective, so up to you.
What's more straightforward and readable for one, might not be the same for the other :)

bmbouter · 2019-05-16T15:55:10Z

docs/workflows/copy.rst

+Copy workflow
+-------------
+
+You can use the copy endpoint to copy RPM-related content present in one repository / repository version to another repository. If you specify a repository, then the latest repository version for that repository will be used.


nitpick. Can all the text lines be line wrapped at 100 chars? It's ok to me if some of the links or quoted items go over 100 lines.

bmbouter · 2019-05-16T15:55:32Z

docs/workflows/copy.rst

+Copy workflow
+-------------
+
+You can use the copy endpoint to copy RPM-related content present in one repository / repository version to another repository. If you specify a repository, then the latest repository version for that repository will be used.


Maybe replace / with 'or' ?

bmbouter · 2019-05-16T15:55:55Z

docs/workflows/copy.rst

+       ...
+    }
+
+You can also specify which types of content you would like to copy by providing a value for the "types" parameter. Types that are not listed will not be copied. The supported types are "package" and "errata". For example, this query will copy only errata, and not packages.


Can this paragraph declare the default also?

bmbouter · 2019-05-16T15:56:57Z

pulp_rpm/app/viewsets.py

+
+    Args:
+        source_repo: repository to copy units from
+        dest_repo: repository to copy units to


This also takes source_repo_version right?

bmbouter · 2019-05-16T15:58:50Z

pulp_rpm/app/serializers.py

+                substitution = RPM_PLUGIN_TYPE_CHOICE_MAP.get(t)
+                if not substitution:
+                    raise serializers.ValidationError(_(
+                        "'{}' is an invalid type, please use one of {}".format(


This won't translate correctly because some languages possibly reorder items. Only name based replacement is safe for string formatting for translation strings.

In this case this probably won't matter, but I try to follow the practice.

bmbouter

@dralley I read through the code and everything looks pretty good to me. I left some requested changes, mostly small things, but hopefully you can look at those and incorporate them with your rebase. I'm lgtm-ing now since the fixes are small. Also would you be willing to file a copy story for QE so they know how to test this? Thank you!

nixocio · 2019-05-16T21:02:45Z

@bmbouter, @dralley already filed a test case https://pulp.plan.io/issues/4721.

As soon as this one is merged we can work on it.

dralley · 2019-05-20T14:05:57Z

Now that I'm back from Summit and being sick I'm going to re-focus on getting this merged. Hopefully today.

daviddavis

LGTM

Also moved upload under app/tasks, because it's a task closes #4716 https://pulp.plan.io/issues/4716

dralley force-pushed the upload-support branch 2 times, most recently from 84ffad7 to 486f3f1 Compare April 20, 2019 05:41

dralley commented Apr 22, 2019

View reviewed changes

bmbouter reviewed Apr 22, 2019

View reviewed changes

bmbouter requested changes Apr 22, 2019

View reviewed changes

dralley force-pushed the upload-support branch 5 times, most recently from 44b0a34 to eafade8 Compare April 25, 2019 20:30

dralley force-pushed the upload-support branch 2 times, most recently from b1b9fa4 to 913a4cb Compare April 26, 2019 03:21

ipanova reviewed Apr 26, 2019

View reviewed changes

dralley commented Apr 26, 2019

View reviewed changes

goosemania reviewed Apr 26, 2019

View reviewed changes

dralley force-pushed the upload-support branch from 913a4cb to c476381 Compare April 26, 2019 14:01

goosemania reviewed Apr 26, 2019

View reviewed changes

dralley force-pushed the upload-support branch from c476381 to 53b77f0 Compare April 26, 2019 14:10

goosemania reviewed Apr 26, 2019

View reviewed changes

dralley force-pushed the upload-support branch 2 times, most recently from c8fd4b9 to 84b4203 Compare April 26, 2019 14:23

goosemania reviewed Apr 26, 2019

View reviewed changes

dralley force-pushed the upload-support branch 3 times, most recently from c17bd4d to 3e3e453 Compare May 1, 2019 01:58

bmbouter reviewed May 16, 2019

View reviewed changes

bmbouter approved these changes May 16, 2019

View reviewed changes

dralley force-pushed the upload-support branch 4 times, most recently from 7ca0a2f to 040eeb4 Compare May 23, 2019 18:35

daviddavis approved these changes May 23, 2019

View reviewed changes

dralley force-pushed the upload-support branch from 040eeb4 to 4efe888 Compare May 23, 2019 18:52

Add content copy feature

0298593

Also moved upload under app/tasks, because it's a task closes #4716 https://pulp.plan.io/issues/4716

dralley force-pushed the upload-support branch from 4efe888 to 0298593 Compare May 23, 2019 18:54

dralley merged commit 672769e into pulp:master May 23, 2019

dralley deleted the upload-support branch May 23, 2019 19:45


		You can also specify which types of content you would like to copy by providing a value for the "types" parameter. Types that are not listed will not be copied. The supported types are "packages" and "errata". For example, this query will copy only errata, and not packages.

		``http POST http://localhost:24817/pulp/api/v3/rpm/copy/ source_repo=${SRC_REPO_HREF} dest_repo=${DEST_REPO_HREF} types:="[\"errata\"]"``

Add content copy feature #1328

Add content copy feature #1328

Conversation

dralley commented Apr 20, 2019

dralley commented Apr 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmbouter left a comment

Choose a reason for hiding this comment

nixocio commented Apr 22, 2019

nixocio commented Apr 22, 2019

dralley commented Apr 22, 2019

pep8speaks commented Apr 25, 2019 • edited

Comment last updated at 2019-05-23 18:54:07 UTC

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

ipanova commented Apr 26, 2019

nixocio commented Apr 26, 2019

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 Apr 26, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dralley May 23, 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

bmbouter left a comment

Choose a reason for hiding this comment

nixocio commented May 16, 2019

dralley commented May 20, 2019 • edited

daviddavis left a comment

Choose a reason for hiding this comment

pep8speaks commented Apr 25, 2019 •

edited

goosemania Apr 26, 2019 •

edited

dralley May 23, 2019 •

edited

dralley commented May 20, 2019 •

edited