Content app auto-distributes latest publication #1328
Conversation
|
Attached issue: https://pulp.plan.io/issues/8760 |
pulpcore/content/handler.py
Outdated
| repo_version = distro.repository.latest_version() | ||
| # Search for publication serving latest_version | ||
| try: | ||
| publication = repo_version.publication_set.earliest("pulp_created") |
There was a problem hiding this comment.
Can the distribution have both a repository and a publication set?
There was a problem hiding this comment.
Yes for now, but in the future no.
There was a problem hiding this comment.
I'm not sure i understand the logic here.
What if we have a repository that needs publications, and a new version has just been created, but the autopublishing is still in progress. Then the content app will attempt to handle this repository version as one without publications, right?
There was a problem hiding this comment.
Yes, you would end up getting a 404 if you tried accessing a file that was only created through a publication. If you were to access a piece of content it would probably work. This means for a time you could access content inside of a repository that would not have been exposed due to publication settings.
There was a problem hiding this comment.
Should we use the latest version that has a publication instead in those cases?
| try: | ||
| versions = repository.versions.all() | ||
| publications = Publication.objects.filter(repository_version__in=versions) | ||
| publication = publications.latest("repository_version", "-pulp_created") |
There was a problem hiding this comment.
Should we add an index for this?
There was a problem hiding this comment.
There was a problem hiding this comment.
I've backed off on this, we should investigate indexes but we should probably try to measure the impact rather than adding them speculatively.
There was a problem hiding this comment.
Added task to keep track of this: https://pulp.plan.io/issues/8777
There was a problem hiding this comment.
I think we want to exclude publications where complete=False as well here.
pulpcore/tests/functional/api/using_plugin/test_distributions.py
Outdated
Show resolved
Hide resolved
|
The code and tests here look good. I'll wait until the index is added and the tiny changelog fix. |
|
I also did some testing, which due to the tests wasn't really needed, but it did test well when I ran this code with pulp_file FWIW. |
| """ | ||
|
|
||
| @classmethod | ||
| def setUpClass(cls): | ||
| """Create class-wide variables.""" | ||
| cls.cfg = config.get_config() | ||
| cls.client = api.Client(cls.cfg) | ||
| cls.file_client = gen_file_client() |
There was a problem hiding this comment.
This looks good to me, but I want to also see the final from @daviddavis if that's ok.
@bmbouter Here is the problem(and assumption) I ran into https://pulp.plan.io/issues/8760#note-2
fixes: #8760
https://pulp.plan.io/issues/8760
Please be sure you have read our documentation on creating PRs:
https://docs.pulpproject.org/contributing/pull-request-walkthrough.html