Add --version option to rpm and file distribution commands#1448
Conversation
mdellweg
left a comment
mdellweg
left a comment
There was a problem hiding this comment.
The cli option part looks nice and clean.
The preprocess_entity in glue has a lot of branches i need to trace for a verdict.
Do you think we should unittest this very function here?
But in any case can you please add a changelog entry, ideally one for cli and glue each?
| def preprocess_entity(self, body: EntityDefinition, partial: bool = False) -> EntityDefinition: | ||
| body = super().preprocess_entity(body, partial=partial) | ||
| if "version" in body: | ||
| self.pulp_ctx.needs_plugin(PluginRequirement("core", specifier=">=3.106.0")) |
There was a problem hiding this comment.
So {"version": None} is supposed to fail on pulp_rpm < 3.106.0?
I wonder if we should just move this statements after line 94.
There was a problem hiding this comment.
Setting repository version on a distribution for a publication based plugin should fail if <3.106
| if not repository_href and partial: | ||
| current_entity = self.entity | ||
| repository_href = current_entity.get("repository") | ||
| if not repository_href and ( | ||
| repository_version_href := current_entity.get("repository_version") | ||
| ): | ||
| repository_href = f"{repository_version_href.rsplit('/', 3)[0]}/" | ||
| if not repository_href: |
There was a problem hiding this comment.
This could be
if repository_href is None:
if partial:
...
repository_href = ...
else:
raise ...| raise PulpException(_("--repository must be provided")) | ||
| body["repository_version"] = f"{repository_href}versions/{version}/" | ||
| body["repository"] = None | ||
| elif "repository" in body and self.pulp_ctx.has_plugin( |
There was a problem hiding this comment.
Is the intention here that when specifying the repository but not the version, the version should be nullified?
So we need the "repository" in body part of that condition?
There was a problem hiding this comment.
There's 2 fields in the CLI (repository and version) which controls which of the 3 fields on distribution get set. If both are present then it sets the repository version, otherwise it sets the repository only.
|
@mdellweg How's this? I simplified it a bit and removed the ability to switch version without re-specifying repository. Commits should be squashed before merging |
mdellweg
left a comment
mdellweg
left a comment
There was a problem hiding this comment.
Just one tiny suggestion to try.
| elif "repository" in body and self.pulp_ctx.has_plugin( | ||
| PluginRequirement("core", specifier=">=3.106.0") | ||
| ): | ||
| body["repository_version"] = None |
There was a problem hiding this comment.
Now that is kind of easy to follow.
Allow setting repository_version on distributions for publication-based plugins (rpm, file), matching the capability added in pulpcore 3.106.0. Assisted-By: Claude Opus 4.6
2 participants
Allow setting repository_version on distributions for publication-based plugins (rpm, file), matching the capability added in pulpcore 3.106.0.
Assisted-By: Claude Opus 4.6