Allow users to specify distribution and component when modifying content - #1492
Allow users to specify distribution and component when modifying content#1492daviddavis wants to merge 1 commit into
Conversation
daviddavis
changed the title
77c51ac to
80bb31b
Compare
72f3350 to
251cab2
Compare
251cab2 to
9f78c10
Compare
Allow repositories/deb/apt/{pulp_id}/modify/ requests to add/remove
packages using optional distribution and component parameters. The task
will create or remove matching release structure content while
preserving package-only behavior when both parameters are omitted.
fixes pulp#1491
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
9f78c10 to
ca95206
Compare
|
First of all: I really appreciate this change! I started by testing some workflows using this, against my own assumptions and expectations. Overall the current state makes very reasonable design choices. It appears to work well (though I have not yet tested the content removal workflows 😉) That being said, on the details, there are several design choices I would like to have some (open ended) discussion on. To avoid this turning to chaotic, I am going to avoid GitHub's review feature for now, and will instead open a thread for each independent thing I would like to discuss. I may need a bit of time to add each thread below. |
| release component, release architecture, and package-release-component content; omitting both | ||
| preserves the existing package-only behavior. No newline at end of file |
There was a problem hiding this comment.
omitting both preserves the existing package-only behavior.
As I mentioned on the issue, I would very much like to follow up with a breaking change at some point that makes the "existing package-only behavior" impossible. It never really made a lot of sense to allow adding packages that are then ignored by the default publish. It is not at all intuitive for new users. However, in the interest of making progress, I think it is best we go ahead as stated here, and postpone the whole discussion about a breaking change for a follow up PR.
| distribution = distribution or PACKAGE_UPLOAD_DEFAULT_DISTRIBUTION | ||
| component = component or PACKAGE_UPLOAD_DEFAULT_COMPONENT |
There was a problem hiding this comment.
This is a bit of a minor technicality, but I do want to discuss it regardless: Defaulting to the "upload defaults", feels a little weird, since modify is not, an "upload" action. I suppose the alternative would be to add two new settings PACKAGE_MODIFY_DEFAULT_DISTRIBUTION, and PACKAGE_MODIFY_DEFAULT_COMPONENT. Which raises the question of: does it make sense to start collecting a separate distribution and component default name for each workflow that needs a default value for distributions or components. If I had simply named the variables PACKAGE_DEFAULT_DISTRIBUTION and PACKAGE_DEFAULT_COMPONENT when I introduced them, it would have been simpler. I may be over thinking this. But it just feels a little weird that using modify results in a default component of 'upload'...
| architecture, _ = ReleaseArchitecture.objects.get_or_create( | ||
| distribution=distribution, architecture=package.architecture | ||
| ) |
There was a problem hiding this comment.
Whatever the wisdom of this existing design, at least for syncs, we never explicitly create a ReleaseArchitecture for architecture=all. Publish will automatically append "all" to the list of architectures in accordance with the spec for the publication format we use. Just for consistency I am leaning towards not creating ReleaseArchitecture objects with architecture=all here either. I am not sure if having them could ever be harmful, but my bias is to err on the side of consistency, perhaps even on the side of "sub-optimal but consistent".
| # A request that only names a component is scoped to the default distribution. | ||
| distribution = data.get("distribution") or PACKAGE_UPLOAD_DEFAULT_DISTRIBUTION | ||
| releases = Release.objects.filter(distribution=distribution) | ||
| repository = self.context.get("repository") | ||
| repository_version = data.get("base_version") or ( | ||
| repository.latest_version() if repository else None | ||
| ) | ||
| added = releases.filter(pk__in=data.get("add_content_units", [])).exists() | ||
| present = repository_version and releases.filter(pk__in=repository_version.content).exists() | ||
| if not (added or present): | ||
| raise DRFValidationError( | ||
| {"distribution": _("This distribution has no Release in the repository.")} | ||
| ) |
There was a problem hiding this comment.
I consider this to be the most important design choice of this new feature.
It is the thing users who want to use the new fields are most likely to trip over.
From the point of view of the publish code, we don't need to demand there be a Release object to go with the distribution we are targeting. If there is a ReleaseComponent with distribution="my-distribution", but no Releae with distribution="my-distribution", then the publish code will simply invent a default Release object: https://github.com/pulp/pulp_deb/blob/main/pulp_deb/app/tasks/publishing.py#L219
So, we could drop this requirement entirely. If users provide a distribution and/or component value to the modify endpoint, we would simply create the repo structure that these values imply. If they don't like the default values in the Release file, they could still add a custom Release object later.
However, I wonder if it makes sense to have some barrier against users accidentally messing up their repositories, for example because they have a typo in the distribution they supplied. (I suppose if you do mess it up, then the weird result would at least be contained within its own repo version which you could simply delete again).
With the current design there is a barrier against providing a weird distribution, but there is no barrier against providing a weird component. So another version of this validation might be to demand you either add or already have present a ReleaseComponent that matches both the distribution and the release you are targeting.
So I currently see three options here:
- Validation based on presence or addition of Release object (current state of the PR)
- No validation at all (you simply get what you ask for, even if what you asked for contains obvious errors).
- Validation based on presence or addition of a ReleaseComponent (I am not going to let you add the packages to a repo structure that is not already part of the repo, and that you have not explicitly asked me to add).
Opinions?
|
I am going to leave it at that for now. Note that while I may have produced a lot of text, all of the above are posed as open questions. Disagreeing with any implied changes is perfectly fine. If, after some discussion, we conclude we want to stay with the current design that is a perfectly reasonable outcome. |
2 participants
Allow
repositories/deb/apt/{pulp_id}/modify/requests to add/remove packages using optionaldistributionandcomponentparameters. The task will create or remove matching release structure content while preserving package-only behavior when both parameters are omitted.fixes #1491