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
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. |
| 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'...
There was a problem hiding this comment.
I kind of wish I could go back in time, and make it so I introduced these variables as:
DEFAULT_DISTRIBUTION = 'pulp'
DEFAULT_COMPONENT = 'default'
Instead of:
PACKAGE_UPLOAD_DEFAULT_DISTRIBUTION = 'pulp'
PACKAGE_UPLOAD_DEFAULT_COMPONENT = 'upload'
There was a problem hiding this comment.
I think DEFAULT_DISTRIBUTION and DEFAULT_COMPONENT make sense. What if we rename them but also continue supporting them (PACKAGE_UPLOAD_*) for a few releases?
There was a problem hiding this comment.
I could see us needing this in more places in the future, so a plugin wide DEFAULT_DISTRIBUTION and DEFAULT_COMPONENT for whenever we need a distribution/component but the user did not provide one makes the most sense to me. Let's introduce these new variables as part of this PR, leaving the existing PACKAGE_UPLOAD_DEFAULT_* variables untouched for now. We can always drop them in favor of the new variables in a follow up.
| 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".
There was a problem hiding this comment.
Having thought about it some more I think the existing design is correct. A ReleaseArchitecture content indicates that a release supports a particular architecture. Architecture: all is not an architecture, but rather a special value with special meaning within APT repo metadata. I am seriously considering a follow up task to make this even more explicit by putting some validation on ReleaseArchitecture that generally prevents creation of a ReleaseArchitecture "all" going forward.
| # 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?
There was a problem hiding this comment.
Typos would be my biggest concern. If someone misspells something, it is not an easy action to undo. In our system, creating a release or component is an explicit step the user must perform so I think it helps to minimize this risk.
That said, we are validating that the release and the component exist before we hand the modify request off to Pulp so we don't have a strong opinion which option we decide here. In fact, I see good arguments for each.
There was a problem hiding this comment.
We discussed this one internally and we came out leaning towards "no validation", that is you get what you asked for whether that is what you wanted or not.
For number 3. we decided this would again ask users to interact with the plumbing we are trying to insulate them from.
For number 1. (validation based on Release, current state) we decided we should not demand the presence of Release objects given they are not technically required.
Overall my thinking is: If we want to move towards making use of this feature mandatory, then it must be as easy to use as possible. And throwing validation errors demanding certain things should already be in the repo (or must be provided with the modify) makes it harder to use/discourages usage.
I do still worry about typos being a very bad experience here. (Also providing one of distribution/component and not understanding about the implication that this causes the other to default to our default value). The thing that mitigates my concern is that the resulting chaos would be contained within its own repository version. Of course, if users start building further versions on top of the broken one before they realize their error, it can quickly become hard to recover from. I would hope that for most users this would only be an issue when they first experiment with the plugin, and will then quickly learn...
On the whole I still lean towards making the feature easy to use even if this increases the risk of typos resulting in bad outcomes.
|
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. |
ca95206 to
07bd0b6
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>
07bd0b6 to
f318641
Compare
|
@quba42 I think I've addressed everything. Please let me know what you think. |
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