Skip to content
This repository has been archived by the owner on Dec 7, 2022. It is now read-only.

Add support for pass-through publications. #3654

Merged
merged 1 commit into from Sep 21, 2018
Merged

Add support for pass-through publications. #3654

merged 1 commit into from Sep 21, 2018

Conversation

jortel
Copy link
Contributor

@jortel jortel commented Sep 20, 2018

https://pulp.plan.io/issues/4020

Adds support for pass-through publications.

Publishers would still use the Publication context and may (optionally) set pass-through=True. In combination with this, the publisher can (optionally) add PublishedMeta and possibly PublishedArtifacts to customize how the publication is composed.

I still need to run EXMPLAIN to see what django has translated the content-artifact query to be sure it's efficient and using the indexes.

I'm not sure where/how to update the docs. I'm inclined to capture pass-through in planned more comprehensive docs rewrite stores.

@pep8speaks
Copy link

Hello @jortel! Thanks for submitting the PR.

@codecov
Copy link

codecov bot commented Sep 20, 2018

Codecov Report

Merging #3654 into master will decrease coverage by 0.2%.
The diff coverage is 27.77%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3654      +/-   ##
=========================================
- Coverage      56%   55.8%   -0.21%     
=========================================
  Files          61      61              
  Lines        2680    2695      +15     
=========================================
+ Hits         1501    1504       +3     
- Misses       1179    1191      +12
Impacted Files Coverage Δ
pulpcore/pulpcore/app/serializers/repository.py 94.7% <100%> (+0.03%) ⬆️
pulpcore/pulpcore/app/models/publication.py 65% <100%> (+1.2%) ⬆️
pulpcore/pulpcore/app/views/content.py 23.52% <13.33%> (-2.64%) ⬇️

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 cebb5de...1c8ef71. Read the comment docs.

@daviddavis
Copy link
Contributor

daviddavis commented Sep 20, 2018

I was thinking that pass_through was going to only be available to plugin writers. I have two concerns about this option be managed by users. First, I wonder if users (as opposed to plugin writers) can understand the intricacies of pass-through vs not. How will they know whether they need to create a pass-through publication vs not? Also, what if I have plugin and I don't want to allow my content to be published via pass-through publications?

Edit: To give an example, in pulp_rpm we generate PublishedMetadata and probably don't want users to create publications via POST /pulp/api/v3/publications/ and bypass the publishing code.

@daviddavis
Copy link
Contributor

daviddavis commented Sep 20, 2018

I think to kind of address the concerns in my previous comment, we could remove the POST /pulp/api/v3/publications/ endpoint from core and instead let plugin writers define their own endpoints to utilize the pass_through option. So for example, the Ansible plugin might define its own POST /pulp/api/v3/ansible/publications endpoint that produces passthrough publications.

@bmbouter
Copy link
Member

This PR is exactly as I imagined it, including the create that only allows pass_through=True. I hadn't considered what @daviddavis was saying though. Now I agree though that we should remove the ability to create Publications via the API and leave it for plugin writers to do via the plugin API. For me it's a safety issue. For plugins where its author expects to only use PublishedMetadata and/or PublishedArtifact, its users cannot use pass_through=True safely.

It's also not needed for the immediate use case. With pulp_ansible the plugin writer is making the Publication via plugin-code so the core API call isn't used there.

@jortel
Copy link
Contributor Author

jortel commented Sep 20, 2018

I will remove the publication POST endpoint and supporting changes.

@daviddavis
Copy link
Contributor

LGTM

Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also looks good to me. Thank you so much @jortel this unblocks a plugin writer w/ pulp_ansible!

@jortel jortel merged commit a18058a into pulp:master Sep 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants