fix: rename ManifestAddArtifactOptions.Annotations to ArtifactAnnotations#28981
fix: rename ManifestAddArtifactOptions.Annotations to ArtifactAnnotations#28981stefan8 wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
kind/api-change
stefan8
force-pushed
the
fix/manifest-add-artifact-annotations-swagger
branch
2 times, most recently
from
cacc494 to
d9984e8
Compare
| LayerType *string `json:"artifact_layer_type,omitempty"` | ||
| ExcludeTitles *bool `json:"artifact_exclude_titles,omitempty"` | ||
| Subject *string `json:"artifact_subject,omitempty"` | ||
| ArtifactAnnotations map[string]string `json:"artifact_annotations,omitempty"` |
There was a problem hiding this comment.
We cannot change the types in pkg/bindings as this would be a breaking change for all consumers of the package. pkg/bindings is also meant to be uses by third parties to talk to the podman API so we need to keep it stable
There was a problem hiding this comment.
Understood, I reverted the pkg/bindings changes so the public API is unchanged — the rename is now only in the entities ManifestAddArtifactOptions struct, which is what the swagger:model is generated from, so that's enough to drop the duplicate x-go-name.
The pr-should-include-tests check is red because this is a go-symbol rename only, so there's no new behavior to test - the existing artifact_annotations coverage in test/apiv2/15-manifest.at already exercises the field.
…ions The swagger:model ManifestAddArtifactOptions (pkg/domain/entities) embeds ManifestAnnotateOptions, which already has an Annotations field, and also declared its own Annotations field. The generated swagger therefore emitted two members with the same x-go-name, so Go clients generated from the spec failed to compile (the reproducer on the issue). Rename the field in the entities struct to ArtifactAnnotations, matching the sibling ManifestModifyOptions. The json/schema tags stay "artifact_annotations", so the REST API wire format is unchanged. pkg/bindings is intentionally left untouched: it is part of the public client API used by third parties, so its option/field names must stay stable. swagger.yaml is generated by make swagger from these annotations and is left for CI to regenerate. Fixes: podman-container-tools#22966 Signed-off-by: Grzegorz Szczepanczyk <g.szczepanczyk@getprintbox.com>
stefan8
force-pushed
the
fix/manifest-add-artifact-annotations-swagger
branch
from
d9984e8 to
34121d4
Compare
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
Honny1
added
the
No New Tests
| LayerType: opts.LayerType, | ||
| ExcludeTitles: opts.ExcludeTitles, | ||
| Annotations: opts.Annotations, | ||
| Annotations: opts.ArtifactAnnotations, |
There was a problem hiding this comment.
Is this correct? It will set ArtifactAnnotation into the embedded field Annotations.
There was a problem hiding this comment.
Yes, correct. The Annotations: field on this line belongs to libimage.ManifestListAddArtifactOptions (the addArtifactOptions literal at L358), not to the embedded entities.ManifestAnnotateOptions. So it sets the artifact's annotations — the same destination they had before this PR.
The value is unchanged: before the rename, ManifestAddArtifactOptions had a direct Annotations field that shadowed the embedded ManifestAnnotateOptions.Annotations (both map[string]string), so opts.Annotations here already resolved to the artifact field (json artifact_annotations). The rename keeps that json/schema tag, so opts.ArtifactAnnotations is the identical field — only the Go symbol is now unambiguous.
The manifest-list (index) annotations travel a separate path and are untouched: they're read via the fully-qualified opts.ManifestAnnotateOptions.Annotations at L382 into ManifestListAnnotateOptions. That qualified access already existed precisely because the direct field used to shadow the embedded one, so the rename can't orphan or misroute it.
Net: artifact annotations → artifact options (this line); manifest annotations → annotate options (L382). The rename only removes the duplicate x-go-name: Annotations in the generated swagger model — no behavior change.
| options.WithConfig(opts.Config) | ||
| options.WithExcludeTitles(opts.ExcludeTitles).WithSubject(opts.Subject) | ||
| options.WithAnnotations(opts.Annotations) | ||
| options.WithAnnotations(opts.ArtifactAnnotations) |
There was a problem hiding this comment.
Same reasoning — WithAnnotations(opts.ArtifactAnnotations) sets the artifact-add binding's annotations. It previously read opts.Annotations, which via the shadowing above was the same artifact field, so the value is identical (see my reply on abi/manifest.go:364).
Honny1
left a comment
Honny1
left a comment
There was a problem hiding this comment.
LGTM
PTAL @podman-container-tools/podman-reviewers @podman-container-tools/podman-maintainers
4 participants
The artifact-specific
Annotationsfield onManifestAddArtifactOptionscollided (same swaggerx-go-name) with the embeddedManifestAnnotateOptions.Annotations, so Go clients generated from the spec fail to compile. This renames it toArtifactAnnotations(the name the sibling options struct already uses for that JSON tag); the wire format is unchanged.Checklist
git commit -s).Fixes: #22966in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?