fix(publish): warn when push falls back from OCI 1.1 to OCI 1.0 - #14146
fix(publish): warn when push falls back from OCI 1.1 to OCI 1.0#14146htoyoda18 wants to merge 3 commits into
Conversation
Signed-off-by: hiroto.toyoda <hiroto.toyoda@dena.com>
There was a problem hiding this comment.
🟢 Approval recommended
The change is small and localized, with only minor wording/doc clarity issues noted.
Pull request overview
This PR makes the publish flow transparent when registries reject OCI 1.1 manifests by having internal/oci.PushManifest report which OCI version was ultimately used, and emitting a warning from pkg/compose/publish when an automatic fallback to OCI 1.0 occurs.
Changes:
- Extend
internal/oci.PushManifestto return the OCI version used (in addition to the manifest descriptor). - Update the publish path to log a warning when the default OCI 1.1 push falls back to OCI 1.0.
File summaries
| File | Description |
|---|---|
| pkg/compose/publish.go | Captures the OCI version used during push and warns when falling back to OCI 1.0. |
| internal/oci/push.go | Returns the OCI version used from PushManifest, including in the fallback path. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // PushManifest pushes the manifest for a Compose OCI artifact and returns | ||
| // the OCI version actually used. | ||
| func PushManifest(ctx context.Context, resolver remotes.Resolver, named reference.Named, layers []v1.Descriptor, ociVersion api.OCIVersion) (v1.Descriptor, api.OCIVersion, error) { |
| if options.OCIVersion == "" && usedOCIVersion == api.OCIVersion1_0 { | ||
| logrus.Warn("registry does not support OCI 1.1 artifacts; falling back to OCI 1.0 format") | ||
| } |
glours
left a comment
There was a problem hiding this comment.
PushManifest currently returns the raw OCI version it ended up using (api.OCIVersion), and the caller reconstructs "did we fall back?" by comparing two independent fields:
if options.OCIVersion == "" && usedOCIVersion == api.OCIVersion1_0 {
logrus.Warn(...)
}Returning a bool (e.g. didFallback) instead would be a better fit for what's actually needed right now:
PushManifestis the only place that knows why it picked a given version — the moment it decides to retry withapi.OCIVersion1_0is exactly the fallback event itself. Aboolcaptures that fact directly, at the source, instead of asking the caller to re-derive it from two values that happen to correlate today.- The caller only ever needs a yes/no signal to decide whether to warn — it never uses the specific
api.OCIVersionvalue for anything else. A richer return type is carrying more information than any consumer reads. - It removes a hidden coupling: today, if a second fallback tier were ever added (e.g. 1.1 → 1.0 → some legacy format), the caller's
usedOCIVersion == api.OCIVersion1_0check would need to be updated in lockstep to keep detecting it as a fallback — and nothing would force that update, since it'd still compile and just silently stop warning on the new tier. Aboolset at the fallback branch itself doesn't have that failure mode.
What I did
PushManifestsilently retried in OCI 1.0 format whenever a registry rejected the OCI 1.1 manifest, leaving users unaware their artifact wasn't stored in the newer format. This was left as a TODO becauseinternal/ociintentionally avoids importinglogrus.PushManifestnow reports which OCI version was actually used instead of just success/failure, andpkg/compose/publish(which already depends onlogrus) logs a warning when that differs from what was requested.Related issue
N/A
(not mandatory) A picture of a cute animal, if possible in relation to what you did
🐈🐈🐈