Remove RepositoryPublishURLSerializer and use PublicationSerializer #103

daviddavis · 2019-04-26T13:23:49Z

refs #4678
https://pulp.plan.io/issues/4678

Please be sure you have read our documentation on creating PRs:
https://docs.pulpproject.org/en/3.0/nightly/contributing/pull-request-walkthrough.html

pulpcore/app/serializers/publication.py

RequiredPR: pulp/pulpcore-plugin#93 RequiredPR: pulp/pulpcore#103 https://pulp.plan.io/issues/4699 fixes #4699

ref #4678 Required PR: pulp/pulpcore#103 Required PR: https://github.com/pulp/pulp_file

asmacdo · 2019-04-26T14:10:53Z

pulpcore/tests/functional/api/using_plugin/constants.py

 FILE_PUBLICATION_PATH = urljoin(BASE_PUBLICATION_PATH, 'file/file/')

+FILE_PUBLISHER_PATH = urljoin(BASE_PUBLISHER_PATH, 'file/file/')


I appreciate this :)

nixocio

@rochacbruno, any comment?

ref #4678 Required PR: pulp/pulpcore#103 Required PR: pulp/pulp_file#207

codecov · 2019-04-26T16:55:17Z

Codecov Report

Merging #103 into master will decrease coverage by 0.04%.
The diff coverage is 94.44%.

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
- Coverage   67.83%   67.79%   -0.05%     
==========================================
  Files          65       65              
  Lines        3019     3015       -4     
==========================================
- Hits         2048     2044       -4     
  Misses        971      971

Impacted Files	Coverage Δ
pulpcore/app/serializers/__init__.py	`100% <ø> (ø)`	⬆️
pulpcore/app/serializers/repository.py	`100% <ø> (+1.02%)`	⬆️
pulpcore/app/serializers/publication.py	`98.88% <94.44%> (-1.12%)`	⬇️
pulpcore/app/viewsets/task.py	`97.01% <0%> (-1.5%)`	⬇️
pulpcore/app/models/publication.py	`72% <0%> (+1.33%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ad7ddf...173de7f. Read the comment docs.

nixocio · 2019-04-26T17:09:11Z

@daviddavis, we do have an extra step to test generated bindings, and this step is failing.

daviddavis · 2019-04-26T17:12:27Z

@kersommoura yes, the problem is there's no way to require a bindings PR here AFAICT.

pulpcore/app/serializers/publication.py

bmbouter · 2019-04-29T15:14:53Z

pulpcore/tests/functional/api/using_plugin/utils.py

@@ -69,3 +70,28 @@ def gen_file_remote(url=None, **kwargs):
        url = FILE_FIXTURE_MANIFEST_URL

    return gen_remote(url, **kwargs)
+
+
+def publish(cfg, repo, version_href=None, publisher=None):


Do we want this to come from pulp_smash.pulp3.utils or here?
Should this still be publish? It seems like it should be create_publication or gen_publication or ?

I didn't move this into pulp_smash.pulp3.utils because this is strictly for the file plugin. Each plugin will create its publication a bit differently with a different endpoint, params, etc so I don't think it can or should be generalized and moved into pulp_smash.pulp3.utils. I can update it to create_publication.

bmbouter · 2019-04-29T15:50:42Z

pulpcore/tests/functional/api/using_plugin/utils.py

@@ -69,3 +70,28 @@ def gen_file_remote(url=None, **kwargs):
        url = FILE_FIXTURE_MANIFEST_URL

    return gen_remote(url, **kwargs)
+
+
+def create_publication(cfg, repo, version_href=None, publisher=None):


Does it make any sense to import this from here?

Or maybe instead call it a generic publication? I think that's part of my confusion is that this is in pulpcore but it says it's specific to the file plugin.

So currently I can run pulpcore's smash tests without pulp_file installed. It just skips over the using_plugin smash tests. If I try to import this function from pulp_file and I run the smash tests, I get ModuleNotFoundError: No module named 'pulp_file'.

We could import guard it to avoid that issue. Since they will be skipped that is ok. I think we should avoid duplicating this create_publication code into all the plugins. What do you think?

This code won't go into all plugins. It's specific to the file plugin. Each plugin will have its own publication path, params, etc and thus will require its own create_publication function.

I was talking about the duplication of the one from pulp_file specifically. It's duplicated here, and again in pulp-certguard.

After some IRC discussion we are going to leave this here and not import from pulp_file.

RequiredPR: pulp/pulpcore-plugin#93 RequiredPR: pulp/pulpcore#103 https://pulp.plan.io/issues/4699 fixes #4699

bmbouter

This all looks good to me. Thanks @daviddavis 🦅 🍰 🎆

Required PR: pulp/pulpcore-plugin#93 Required PR: pulp/pulp_file#207 Required PR: pulp/pulp-certguard#19 ref pulp#4678 https://pulp.plan.io/issues/4678

daviddavis force-pushed the issue4678 branch 2 times, most recently from 769f3df to f9c8a8e Compare April 26, 2019 13:28

asmacdo reviewed Apr 26, 2019

View reviewed changes

pulpcore/app/serializers/publication.py Outdated Show resolved Hide resolved

asmacdo reviewed Apr 26, 2019

View reviewed changes

pulpcore/app/serializers/publication.py Outdated Show resolved Hide resolved

daviddavis force-pushed the issue4678 branch 2 times, most recently from 004a3a8 to 1ebe344 Compare April 26, 2019 13:38

asmacdo added a commit to asmacdo/pulp_python that referenced this pull request Apr 26, 2019

Swith to Master/Detail Publications

ee5370e

RequiredPR: pulp/pulpcore-plugin#93 RequiredPR: pulp/pulpcore#103 https://pulp.plan.io/issues/4699 fixes #4699

asmacdo mentioned this pull request Apr 26, 2019

A Publication create needs repository #102

Closed

daviddavis force-pushed the issue4678 branch from 1ebe344 to 26e9688 Compare April 26, 2019 14:07

daviddavis requested a review from a team April 26, 2019 14:07

daviddavis pushed a commit to daviddavis/pulpcore-plugin that referenced this pull request Apr 26, 2019

Remove RepositoryPublishURLSerializer

dc0cdaa

ref #4678 Required PR: pulp/pulpcore#103 Required PR: https://github.com/pulp/pulp_file

asmacdo reviewed Apr 26, 2019

View reviewed changes

daviddavis force-pushed the issue4678 branch 3 times, most recently from 14195e9 to 0942d69 Compare April 26, 2019 16:09

nixocio approved these changes Apr 26, 2019

View reviewed changes

daviddavis force-pushed the issue4678 branch from 0942d69 to b57cb38 Compare April 26, 2019 16:40

daviddavis pushed a commit to daviddavis/pulpcore-plugin that referenced this pull request Apr 26, 2019

Remove RepositoryPublishURLSerializer

56e9afd

ref #4678 Required PR: pulp/pulpcore#103 Required PR: pulp/pulp_file#207

daviddavis force-pushed the issue4678 branch 2 times, most recently from 4de9eda to 01455ce Compare April 27, 2019 16:22

bmbouter reviewed Apr 29, 2019

View reviewed changes

pulpcore/app/serializers/publication.py Outdated Show resolved Hide resolved

bmbouter reviewed Apr 29, 2019

View reviewed changes

daviddavis force-pushed the issue4678 branch from 01455ce to d13c987 Compare April 29, 2019 15:29

bmbouter reviewed Apr 29, 2019

View reviewed changes

asmacdo added a commit to asmacdo/pulp_python that referenced this pull request Apr 29, 2019

Swith to Master/Detail Publications

66b8e8b

RequiredPR: pulp/pulpcore-plugin#93 RequiredPR: pulp/pulpcore#103 https://pulp.plan.io/issues/4699 fixes #4699

bmbouter approved these changes Apr 29, 2019

View reviewed changes

daviddavis force-pushed the issue4678 branch from d13c987 to 3886e53 Compare April 29, 2019 18:29

Remove RepositoryPublishURLSerializer and use PublicationSerializer

173de7f

Required PR: pulp/pulpcore-plugin#93 Required PR: pulp/pulp_file#207 Required PR: pulp/pulp-certguard#19 ref pulp#4678 https://pulp.plan.io/issues/4678

daviddavis force-pushed the issue4678 branch from 3886e53 to 173de7f Compare April 29, 2019 18:38

dkliban merged commit 5dd64f6 into pulp:master Apr 29, 2019

daviddavis deleted the issue4678 branch April 29, 2019 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove RepositoryPublishURLSerializer and use PublicationSerializer #103

Remove RepositoryPublishURLSerializer and use PublicationSerializer #103

daviddavis commented Apr 26, 2019

asmacdo Apr 26, 2019

nixocio left a comment

codecov bot commented Apr 26, 2019 •

edited

nixocio commented Apr 26, 2019

daviddavis commented Apr 26, 2019

bmbouter Apr 29, 2019

daviddavis Apr 29, 2019

bmbouter Apr 29, 2019

bmbouter Apr 29, 2019

daviddavis Apr 29, 2019

bmbouter Apr 29, 2019

daviddavis Apr 29, 2019

bmbouter Apr 29, 2019

bmbouter Apr 29, 2019

bmbouter left a comment

		FILE_PUBLICATION_PATH = urljoin(BASE_PUBLICATION_PATH, 'file/file/')

		FILE_PUBLISHER_PATH = urljoin(BASE_PUBLISHER_PATH, 'file/file/')

Remove RepositoryPublishURLSerializer and use PublicationSerializer #103

Remove RepositoryPublishURLSerializer and use PublicationSerializer #103

Conversation

daviddavis commented Apr 26, 2019

Choose a reason for hiding this comment

nixocio left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 26, 2019 • edited

Codecov Report

nixocio commented Apr 26, 2019

daviddavis 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

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

codecov bot commented Apr 26, 2019 •

edited