From 5e30d834c9b1a6fb8f06cf97ae4f82e576dcf5c0 Mon Sep 17 00:00:00 2001 From: Maximilian Date: Mon, 27 Apr 2026 13:26:51 +0200 Subject: [PATCH] fix(phpup): publish bundles under canonical tags, propagate annotations phpup push wrote each promoted manifest to a digest-prefix tag like 'sha256-' instead of the canonical '----' shape. Bundles landed on GHCR but were unreachable to lockfile-update, which queries the canonical tag and got 404, silently dropping the entry from bundles.lock. Switch PushAll to ListKeyed (which exposes the source manifest's io.buildrush.bundle.key annotation), derive the canonical (image-name, tag) pair from the key via canonicalRefFromBundleKey, and propagate BundleKey + SpecHash annotations to the destination so LookupBySpec also resolves on the promoted remote. Reproduces the failure that left main with redis 6.3.0 in the catalog but no 6.3.0 entries in bundles.lock or canonical GHCR tags after PR #76 merged: phpup push pushed redis 6.3.0 blobs+manifests under sha256-* tags, lockfile-update HEAD'd 'redis:6.3.0-8.5-nts-linux-x86_64' and got 404, and the canonical-tag listing on GHCR remained 6.2.0-only. Comment on the previous deriveTagForPromotion explicitly flagged this as a 'PR 6 will fix' stub that never landed. Updates push_test.go to assert canonical tags + propagated annotations, adds TestCanonicalRefFromBundleKey covering core/ext/error shapes, and fixes a stale ext-key example in registry/annotations.go that used the defunct '8.4-nts' joined PHPAbi format. --- internal/build/push.go | 129 ++++++++-------- internal/build/push_test.go | 256 ++++++++++++++++++------------- internal/registry/annotations.go | 2 +- 3 files changed, 217 insertions(+), 170 deletions(-) diff --git a/internal/build/push.go b/internal/build/push.go index 8637388..e8ccd44 100644 --- a/internal/build/push.go +++ b/internal/build/push.go @@ -5,6 +5,7 @@ import ( "errors" "flag" "fmt" + "strings" "github.com/buildrush/setup-php/internal/registry" ) @@ -25,25 +26,19 @@ func PushMain(args []string) error { return PushAll(context.Background(), *from, *to) } -// PushAll promotes every bundle in source to dest. Source MUST be an -// oci-layout (remote->remote copy isn't supported in PR 4; use oras for -// that case) — a remote source is rejected up front with an error -// mentioning the supported URI form. +// PushAll promotes every keyed bundle in source to dest. Source MUST be +// an oci-layout populated by `phpup build`, where each manifest carries +// the `io.buildrush.bundle.key` annotation — that's what lets us recover +// the canonical publication tag (e.g. "6.3.0-8.5-nts-linux-x86_64" for +// `ext:redis:6.3.0:8.5:linux:x86_64:nts`) on the destination. // -// The walk reads the source layout's index, and for each manifest -// re-pushes the layer bytes + meta to the destination via Store.Push. -// Tag derivation is currently lossy: the source manifest's original -// publication tag isn't recoverable from Fetch, so deriveTagForPromotion -// falls back to a digest-prefix tag like "sha256-abc123…". PR 6 will -// carry the original tag via a manifest annotation. +// Manifests without a bundle-key annotation are skipped silently by +// ListKeyed; the `phpup build` path always sets it. Forward push paths +// only see build-produced layouts, so this exposure is bounded. // -// The bundle-name annotation is preserved (via Annotations.BundleName) -// so the destination still resolves through LookupBySpec / Has for -// downstream consumers. The source-side spec-hash is not propagated — -// Fetch doesn't surface manifest annotations, so the promoted manifest -// ends up annotated only by what ref.Name implies. When/if LookupBySpec -// on a promoted remote becomes required, the Store interface will need -// an annotations-accessor. +// The destination manifest is annotated with BundleName/BundleKey/SpecHash +// so consumers can resolve via canonical tag (lockfile-update) AND via +// LookupBySpec (build-cache probe). func PushAll(ctx context.Context, sourceURI, destURI string) error { src, err := registry.Open(sourceURI) if err != nil { @@ -53,75 +48,77 @@ func PushAll(ctx context.Context, sourceURI, destURI string) error { if err != nil { return fmt.Errorf("phpup push: open dest %s: %w", destURI, err) } - // Source MUST expose an index walk. Only the layout backend does so - // natively in PR 4; a remote source would need a custom API to - // enumerate tagged manifests (deferred to PR 6 consolidation). + // Source MUST expose ListKeyed so we can recover the canonical tag + // from each manifest's bundle-key annotation. Only the layout backend + // implements it natively; a remote source would need a custom API to + // enumerate annotated manifests. lister, ok := src.(interface { - List(ctx context.Context) ([]registry.Ref, error) + ListKeyed(ctx context.Context) ([]registry.KeyedRef, error) }) if !ok { - return fmt.Errorf("phpup push: source %q doesn't support index walk (use oci-layout:)", sourceURI) + return fmt.Errorf("phpup push: source %q doesn't support ListKeyed (use oci-layout:)", sourceURI) } - refs, err := lister.List(ctx) + refs, err := lister.ListKeyed(ctx) if err != nil { return fmt.Errorf("phpup push: list source: %w", err) } fmt.Printf("phpup push: promoting %d manifests from %s to %s\n", len(refs), sourceURI, destURI) - for _, ref := range refs { - if err := promoteOne(ctx, src, dst, ref); err != nil { - return fmt.Errorf("phpup push: promote %s: %w", ref, err) + for _, kref := range refs { + if err := promoteOne(ctx, src, dst, kref); err != nil { + return fmt.Errorf("phpup push: promote %s: %w", kref.Key, err) } } return nil } -// promoteOne fetches a single source manifest's bundle + meta and -// pushes the payload to the destination under a derived tag. Close -// failures on the source reader are swallowed — the push has already -// succeeded at that point and the caller's next action is typically to -// print a success line and exit. -func promoteOne(ctx context.Context, src, dst registry.Store, ref registry.Ref) error { - rc, meta, err := src.Fetch(ctx, ref) +// promoteOne fetches a single source manifest's bundle + meta and pushes +// the payload to the destination under the canonical tag derived from +// the bundle key. The destination manifest carries the propagated +// BundleKey + SpecHash annotations so post-push lockfile-update and +// LookupBySpec both resolve correctly. +func promoteOne(ctx context.Context, src, dst registry.Store, kref registry.KeyedRef) error { + imageName, tag, err := canonicalRefFromBundleKey(kref.Key) + if err != nil { + return err + } + rc, meta, err := src.Fetch(ctx, registry.Ref{Name: kref.Name, Digest: kref.Digest}) if err != nil { return fmt.Errorf("fetch: %w", err) } defer func() { _ = rc.Close() }() - tag := deriveTagForPromotion(ref) - if tag == "" { - return fmt.Errorf("couldn't derive tag for %s (ref has no digest)", ref) - } - if ref.Name == "" { - return fmt.Errorf("couldn't derive name for %s (ref has no bundle-name annotation)", ref) + dstRef := registry.Ref{Name: imageName, Tag: tag} + ann := registry.Annotations{ + BundleName: imageName, + BundleKey: kref.Key, + SpecHash: kref.SpecHash, } - dstRef := registry.Ref{Name: ref.Name, Tag: tag} - ann := registry.Annotations{BundleName: ref.Name} return dst.Push(ctx, dstRef, rc, meta, ann) } -// deriveTagForPromotion produces a tag string for a manifest based on -// its digest. The canonical publication tag shape -// (e.g. "---" for php-core) isn't recoverable -// from the Fetch return — go-containerregistry's layout reader doesn't -// expose per-manifest tags on the index, and the digest alone doesn't -// encode them. For PR 4 we fall back to "sha256-" so the -// destination registry has SOMETHING to address, at the cost of losing -// human-readable tag semantics. PR 6's CLI consolidation will thread -// the original tag via the source manifest's annotation so promotions -// stay addressable under their published names. -func deriveTagForPromotion(ref registry.Ref) string { - if ref.Digest == "" { - return "" - } - // "sha256:" prefix is 7 characters; the hex payload that follows is - // 64 characters for sha256. Take 12 hex chars after the prefix — - // enough to avoid collisions in any realistic layout, short enough - // to stay within the OCI tag length limit (128 chars). - d := ref.Digest - if len(d) > 19 { - return "sha256-" + d[7:19] +// canonicalRefFromBundleKey parses a bundle key produced by +// internal/lockfile.{PHPBundleKey,ExtBundleKey} and returns the +// (image-name, tag) pair that mirrors what `phpup lockfile-update` +// queries against ghcr.io. The two paths must agree for `phpup push` → +// `lockfile-update` to round-trip. +// +// php:::: +// → ("php-core", "---") +// ext:::::: +// → ("php-ext-", "----") +// +// Tool keys aren't supported yet — `phpup build` doesn't produce them +// and `lockfile-update` doesn't query them. Returns an explicit error +// rather than guessing a layout. +func canonicalRefFromBundleKey(key string) (imageName, tag string, err error) { + parts := strings.Split(key, ":") + switch { + case len(parts) == 5 && parts[0] == "php": + ver, osName, arch, ts := parts[1], parts[2], parts[3], parts[4] + return "php-core", fmt.Sprintf("%s-%s-%s-%s", ver, osName, arch, ts), nil + case len(parts) == 7 && parts[0] == "ext": + name, ver, phpver, osName, arch, ts := parts[1], parts[2], parts[3], parts[4], parts[5], parts[6] + return "php-ext-" + name, fmt.Sprintf("%s-%s-%s-%s-%s", ver, phpver, ts, osName, arch), nil + default: + return "", "", fmt.Errorf("canonicalRefFromBundleKey: unrecognized bundle key shape %q", key) } - // Pathological fallback — ref.Digest looks like "sha256:" - // which shouldn't happen in practice but we keep the behavior - // defined rather than panicking. - return "sha256-" + d[7:] } diff --git a/internal/build/push_test.go b/internal/build/push_test.go index 33a1136..1359512 100644 --- a/internal/build/push_test.go +++ b/internal/build/push_test.go @@ -32,10 +32,11 @@ func startPushTestRegistry(t *testing.T) string { return u.Host } -// seedLayoutForPush writes a bundle into a fresh oci-layout at -// /layout, then returns the URI. Used to populate a source layout -// for PushAll round-trip tests. -func seedLayoutForPush(t *testing.T, dir, bundleName string, body []byte, meta *registry.Meta) string { +// seedKeyedLayoutForPush writes a bundle into a fresh oci-layout at +// /layout with full annotations (name + key + spec_hash) so the +// PushAll path can recover the canonical tag from the key. Mirrors what +// `phpup build php|ext` writes for produced bundles. +func seedKeyedLayoutForPush(t *testing.T, dir, bundleName, bundleKey, specHash string, body []byte, meta *registry.Meta) string { t.Helper() layoutDir := filepath.Join(dir, "layout") s, err := registry.Open("oci-layout:" + layoutDir) @@ -45,31 +46,38 @@ func seedLayoutForPush(t *testing.T, dir, bundleName string, body []byte, meta * if err := s.Push(context.Background(), registry.Ref{Name: bundleName}, bytes.NewReader(body), meta, - registry.Annotations{BundleName: bundleName}); err != nil { + registry.Annotations{BundleName: bundleName, BundleKey: bundleKey, SpecHash: specHash}); err != nil { t.Fatalf("seed layout: %v", err) } return "oci-layout:" + layoutDir } // TestPushAll_RoundTrip_LayoutToInProcessRegistry is the happy-path -// end-to-end: seed an oci-layout with two manifests (php-core + one -// ext), run PushAll against an in-process registry as destination, and -// verify both manifests are fetchable on the destination under the -// digest-derived tag. +// end-to-end: seed an oci-layout with two annotated manifests (php-core + +// one ext), run PushAll against an in-process registry as destination, +// and verify both manifests are fetchable on the destination under the +// canonical publication tag derived from the bundle key. Also verifies +// that BundleKey and SpecHash annotations are propagated. func TestPushAll_RoundTrip_LayoutToInProcessRegistry(t *testing.T) { ctx := context.Background() tmp := t.TempDir() phpCoreBytes := []byte("fake php-core bundle") phpExtBytes := []byte("fake php-ext-redis bundle") - // Seed source layout with two bundles. - layoutURI := seedLayoutForPush(t, tmp, "php-core", phpCoreBytes, ®istry.Meta{SchemaVersion: 3, Kind: "php-core"}) - // Second push lands in the same layout path. + const phpKey = "php:8.5:linux:x86_64:nts" + const phpHash = "sha256:phpspecdigesthex0000000000000000000000000000000000000000000000" + const extKey = "ext:redis:6.3.0:8.5:linux:x86_64:nts" + const extHash = "sha256:extspecdigesthex0000000000000000000000000000000000000000000000" + + // Seed source layout with two keyed bundles. + layoutURI := seedKeyedLayoutForPush(t, tmp, "php-core", phpKey, phpHash, phpCoreBytes, + ®istry.Meta{SchemaVersion: 3, Kind: "php-core"}) layoutDir := filepath.Join(tmp, "layout") s, _ := registry.Open("oci-layout:" + layoutDir) if err := s.Push(ctx, registry.Ref{Name: "php-ext-redis"}, - bytes.NewReader(phpExtBytes), ®istry.Meta{SchemaVersion: 3, Kind: "php-ext"}, - registry.Annotations{BundleName: "php-ext-redis"}); err != nil { + bytes.NewReader(phpExtBytes), + ®istry.Meta{SchemaVersion: 3, Kind: "php-ext"}, + registry.Annotations{BundleName: "php-ext-redis", BundleKey: extKey, SpecHash: extHash}); err != nil { t.Fatalf("push ext: %v", err) } @@ -80,63 +88,63 @@ func TestPushAll_RoundTrip_LayoutToInProcessRegistry(t *testing.T) { t.Fatalf("PushAll: %v", err) } - // Re-open source to enumerate digest-derived tags the push would - // have used, so we can fetch back from the destination under the - // same tag. - lister := s.(interface { - List(ctx context.Context) ([]registry.Ref, error) - }) - refs, err := lister.List(ctx) - if err != nil { - t.Fatalf("source List: %v", err) - } - if len(refs) != 2 { - t.Fatalf("source layout has %d manifests, want 2", len(refs)) - } - - // For each seeded bundle, verify the destination has a manifest at - // the derived tag with the same layer 0 bytes. - byName := map[string][]byte{ - "php-core": phpCoreBytes, - "php-ext-redis": phpExtBytes, + // Each seeded bundle should land at the canonical tag derived from + // its bundle key — the same tag shape `lockfile-update` queries. + cases := []struct { + key string + wantName string + wantTag string + wantBody []byte + }{ + {phpKey, "php-core", "8.5-linux-x86_64-nts", phpCoreBytes}, + {extKey, "php-ext-redis", "6.3.0-8.5-nts-linux-x86_64", phpExtBytes}, } - for _, r := range refs { - tag := deriveTagForPromotion(r) - if tag == "" { - t.Fatalf("empty derived tag for %s", r) - } - want, ok := byName[r.Name] - if !ok { - t.Fatalf("unexpected source ref %s", r) - } - dest, err := name.ParseReference(destURI + "/" + r.Name + ":" + tag) - if err != nil { - t.Fatalf("parse dest ref: %v", err) - } - desc, err := remote.Get(dest) - if err != nil { - t.Fatalf("remote.Get %s: %v", dest, err) - } - img, err := desc.Image() - if err != nil { - t.Fatalf("image: %v", err) - } - layers, err := img.Layers() - if err != nil { - t.Fatalf("layers: %v", err) - } - if len(layers) < 1 { - t.Fatalf("manifest has %d layers, want >=1", len(layers)) - } - rc, err := layers[0].Compressed() - if err != nil { - t.Fatalf("layer 0: %v", err) - } - got, _ := io.ReadAll(rc) - _ = rc.Close() - if !bytes.Equal(got, want) { - t.Errorf("bundle %s: got %q, want %q", r.Name, got, want) - } + for _, tc := range cases { + t.Run(tc.wantName, func(t *testing.T) { + dest, err := name.ParseReference(destURI + "/" + tc.wantName + ":" + tc.wantTag) + if err != nil { + t.Fatalf("parse dest ref: %v", err) + } + desc, err := remote.Get(dest) + if err != nil { + t.Fatalf("remote.Get %s: %v (canonical tag missing — phpup push regression)", dest, err) + } + img, err := desc.Image() + if err != nil { + t.Fatalf("image: %v", err) + } + // Verify layer 0 bytes match the seeded body. + layers, err := img.Layers() + if err != nil { + t.Fatalf("layers: %v", err) + } + if len(layers) < 1 { + t.Fatalf("manifest has %d layers, want >=1", len(layers)) + } + rc, err := layers[0].Compressed() + if err != nil { + t.Fatalf("layer 0: %v", err) + } + got, _ := io.ReadAll(rc) + _ = rc.Close() + if !bytes.Equal(got, tc.wantBody) { + t.Errorf("bundle %s: got %q, want %q", tc.wantName, got, tc.wantBody) + } + // Verify destination annotations propagated. + mf, err := img.Manifest() + if err != nil { + t.Fatalf("manifest: %v", err) + } + if got := mf.Annotations["io.buildrush.bundle.key"]; got != tc.key { + t.Errorf("dest bundle.key = %q, want %q", got, tc.key) + } + if got := mf.Annotations["io.buildrush.bundle.name"]; got != tc.wantName { + t.Errorf("dest bundle.name = %q, want %q", got, tc.wantName) + } + if got := mf.Annotations["io.buildrush.bundle.spec-hash"]; got == "" { + t.Errorf("dest bundle.spec-hash missing (want propagated from source)") + } + }) } } @@ -152,8 +160,8 @@ func TestPushAll_RemoteSourceRejected(t *testing.T) { if err == nil { t.Fatal("PushAll with remote source: want error, got nil") } - if !strings.Contains(err.Error(), "doesn't support index walk") { - t.Errorf("err = %v, want containing \"doesn't support index walk\"", err) + if !strings.Contains(err.Error(), "doesn't support ListKeyed") { + t.Errorf("err = %v, want containing \"doesn't support ListKeyed\"", err) } } @@ -179,14 +187,11 @@ func TestPushMain_MissingFlags_Errors(t *testing.T) { } // TestPushMain_EmptyLayoutSucceeds: a source layout that exists but -// holds zero manifests is a legal no-op (symmetry with -// List's ErrNotExist handling). Covers the case where PushAll is -// invoked against a fresh build directory before any bundles have been -// pushed. Must print 0-manifests banner and return nil. +// holds zero manifests is a legal no-op (symmetry with ListKeyed's +// ErrNotExist handling). Covers the case where PushAll is invoked +// against a fresh build directory before any bundles have been pushed. +// Must print 0-manifests banner and return nil. func TestPushMain_EmptyLayoutSucceeds(t *testing.T) { - // An empty directory makes layout.FromPath fail with ErrNotExist on - // the first walk — List returns (nil, nil) for that case, so - // PushAll should successfully iterate over zero refs. host := startPushTestRegistry(t) tmp := t.TempDir() err := PushMain([]string{ @@ -198,36 +203,81 @@ func TestPushMain_EmptyLayoutSucceeds(t *testing.T) { } } -// TestDeriveTagForPromotion_DigestShortPrefix pins the fallback tag -// format PushAll emits when the source manifest has no surface-level -// tag info. Shape: "sha256-" — matches the comment on -// deriveTagForPromotion. If a future PR threads the original tag via a -// manifest annotation, this test will start failing and should be -// updated to the new contract. -func TestDeriveTagForPromotion_DigestShortPrefix(t *testing.T) { +// TestCanonicalRefFromBundleKey covers every key shape the helper accepts +// plus the rejection branch. The expected tag formats here are the same +// strings `lockfile-update` queries against ghcr.io — keep this test in +// lockstep with `internal/lockfileupdate.update.go`'s tag construction. +func TestCanonicalRefFromBundleKey(t *testing.T) { cases := []struct { - name string - digest string - want string + name string + key string + wantName string + wantTag string + wantErrSub string }{ - {"standard sha256", "sha256:abcdef012345678901234567890abcdefabcdefabcdefabcdefabcdefabcdef", "sha256-abcdef012345"}, - {"short digest (pathological)", "sha256:abc", "sha256-abc"}, + { + name: "php core 8.5 linux x86_64 nts", + key: "php:8.5:linux:x86_64:nts", + wantName: "php-core", + wantTag: "8.5-linux-x86_64-nts", + }, + { + name: "php core 8.4 linux aarch64 nts", + key: "php:8.4:linux:aarch64:nts", + wantName: "php-core", + wantTag: "8.4-linux-aarch64-nts", + }, + { + name: "ext redis 6.3.0", + key: "ext:redis:6.3.0:8.5:linux:x86_64:nts", + wantName: "php-ext-redis", + wantTag: "6.3.0-8.5-nts-linux-x86_64", + }, + { + name: "ext xdebug 3.5.1", + key: "ext:xdebug:3.5.1:8.4:linux:aarch64:nts", + wantName: "php-ext-xdebug", + wantTag: "3.5.1-8.4-nts-linux-aarch64", + }, + { + name: "tool key not supported", + key: "tool:composer:2.7.0:any:any", + wantErrSub: "unrecognized bundle key shape", + }, + { + name: "wrong prefix", + key: "container:redis:7.0", + wantErrSub: "unrecognized bundle key shape", + }, + { + name: "ext with too few parts", + key: "ext:redis:6.3.0:8.5:linux", + wantErrSub: "unrecognized bundle key shape", + }, + { + name: "empty", + key: "", + wantErrSub: "unrecognized bundle key shape", + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - got := deriveTagForPromotion(registry.Ref{Digest: tc.digest}) - if got != tc.want { - t.Errorf("deriveTagForPromotion(%q) = %q, want %q", tc.digest, got, tc.want) + gotName, gotTag, err := canonicalRefFromBundleKey(tc.key) + if tc.wantErrSub != "" { + if err == nil || !strings.Contains(err.Error(), tc.wantErrSub) { + t.Errorf("err = %v, want containing %q", err, tc.wantErrSub) + } + return + } + if err != nil { + t.Fatalf("err = %v, want nil", err) + } + if gotName != tc.wantName { + t.Errorf("name = %q, want %q", gotName, tc.wantName) + } + if gotTag != tc.wantTag { + t.Errorf("tag = %q, want %q", gotTag, tc.wantTag) } }) } } - -// TestDeriveTagForPromotion_EmptyDigest ensures the empty-digest input -// produces an empty string (caller treats that as "can't derive a tag" -// and errors at the promoteOne boundary). -func TestDeriveTagForPromotion_EmptyDigest(t *testing.T) { - if got := deriveTagForPromotion(registry.Ref{}); got != "" { - t.Errorf("deriveTagForPromotion(empty) = %q, want empty", got) - } -} diff --git a/internal/registry/annotations.go b/internal/registry/annotations.go index f5faff8..ac3b493 100644 --- a/internal/registry/annotations.go +++ b/internal/registry/annotations.go @@ -22,7 +22,7 @@ type Annotations struct { SpecHash string // BundleKey is the canonical lockfile key, e.g. - // "php:8.4:linux:x86_64:nts" or "ext:redis:6.2.0:8.4-nts:linux:x86_64". + // "php:8.4:linux:x86_64:nts" or "ext:redis:6.2.0:8.4:linux:x86_64:nts". // Set by phpup build php|ext; read by phpup test's lockfile-override // synthesis so test containers can resolve bundles against a local // layout whose digests differ from the embedded bundles.lock.