Conversation
…dSnorkel#7) Sometimes /tmp doesn't have enough space. Allow overriding that path with $TMPDIR. Example error: {"level":"error","error":"errors encountered while building soci layers; write /tmp/tmp.3943925700: no space left on device; write /tmp/tmp.1788043098: no space left on device","RegistryURL":"<accountid>.dkr.ecr.<region>.amazonaws.com","ImageDigest":"<digest>","time":"2024-09-30T03:22:06Z","message":"SOCI index build error"}
Hopefully this will fix the automatic generation of changelogs.
Index v1 is being deprecated BREAKING CHANGE: you can no longer index an image but its digest, but only by a tag Based on awslabs/cfn-ecr-aws-soci-index-builder@684544c
Also allow pointing to sha256 digest when --new-tag is used. Fixes CloudSnorkel#16
There was a problem hiding this comment.
Pull request overview
This pull request synchronizes changes from an upstream fork, bringing in significant updates including AWS SDK v2 migration, multi-architecture image support, new tagging functionality, and various dependency updates.
Changes:
- Migrated from AWS SDK v1 to v2 for ECR authentication
- Added support for multi-architecture images with new
GetImageDigestsmethod - Implemented tagging functionality via new
--new-tagflag andTagmethod - Updated Go dependencies and build tooling (goreleaser v2, GitHub Actions)
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Updated Go version and migrated to AWS SDK v2, updated dependencies |
| go.sum | Dependency checksums with unresolved merge conflicts |
| utils/registry/registry.go | AWS SDK v2 migration, new Tag and GetImageDigests methods, custom Manifest type |
| handler.go | Refactored to support multi-digest indexing and tagging workflow |
| main.go | Added image parsing with docker-parser library, new --new-tag flag support |
| main_test.go | New tests for image parsing functionality |
| README.md | Fixed minor punctuation typo |
| .goreleaser.yaml | Updated to goreleaser v2 configuration |
| .github/workflows/goreleaser.yml | Updated to v5 actions and goreleaser v6 |
| .gitattributes | New file enforcing consistent line endings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (registry *Registry) Tag(ctx context.Context, indexDesc ocispec.Descriptor, repositoryName, tag string) error { | ||
| repo, err := registry.registry.Repository(ctx, repositoryName) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| log.Info(ctx, fmt.Sprintf("Tagging index with %s", tag)) | ||
| err = repo.Tag(ctx, indexDesc, tag) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to tag artifact: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The new Tag method lacks test coverage. Since this codebase has established testing practices (as seen in utils/registry/registry_test.go), consider adding tests for this new method to ensure it correctly tags artifacts in the registry.
| return fmt.Errorf("Unexpected config media type: %s, expected one of: %v.", manifest.Config.MediaType, ImageConfigMediaTypes) | ||
| } | ||
|
|
||
| // GetImageDigests inspects an image reference and returns all valid digets that need to be indexed. |
There was a problem hiding this comment.
Typo in comment: "digets" should be "digests".
| // GetImageDigests inspects an image reference and returns all valid digets that need to be indexed. | |
| // GetImageDigests inspects an image reference and returns all valid digests that need to be indexed. |
| } | ||
|
|
||
| if manifest.MediaType == MediaTypeDockerManifestList { | ||
| // multi-arch iamge |
There was a problem hiding this comment.
Typo in comment: "iamge" should be "image".
| // multi-arch iamge | |
| // multi-arch image |
| github.com/opencontainers/runtime-spec v1.3.0 // indirect | ||
| github.com/opencontainers/selinux v1.13.0 // indirect | ||
| github.com/pelletier/go-toml/v2 v2.2.4 // indirect | ||
| github.com/pkg/errors v0.9.1 // indirect | ||
| github.com/rs/xid v1.5.0 // indirect | ||
| github.com/rs/xid v1.6.0 // indirect | ||
| github.com/sirupsen/logrus v1.9.3 // indirect | ||
| github.com/spf13/pflag v1.0.5 // indirect | ||
| go.etcd.io/bbolt v1.3.9 // indirect | ||
| github.com/spf13/pflag v1.0.10 // indirect | ||
| go.etcd.io/bbolt v1.4.3 // indirect | ||
| go.opencensus.io v0.24.0 // indirect | ||
| go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.50.0 // indirect | ||
| go.opentelemetry.io/otel v1.25.0 // indirect | ||
| go.opentelemetry.io/otel/metric v1.25.0 // indirect | ||
| go.opentelemetry.io/otel/trace v1.25.0 // indirect | ||
| golang.org/x/net v0.24.0 // indirect | ||
| golang.org/x/sync v0.7.0 // indirect | ||
| golang.org/x/sys v0.19.0 // indirect | ||
| golang.org/x/text v0.14.0 // indirect | ||
| google.golang.org/genproto v0.0.0-20240415180920-8c6c420018be // indirect | ||
| google.golang.org/genproto/googleapis/rpc v0.0.0-20240415180920-8c6c420018be // indirect | ||
| google.golang.org/grpc v1.63.2 // indirect | ||
| google.golang.org/protobuf v1.33.0 // indirect | ||
| go.opentelemetry.io/auto/sdk v1.2.1 // indirect | ||
| go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.63.0 // indirect | ||
| go.opentelemetry.io/otel v1.38.0 // indirect | ||
| go.opentelemetry.io/otel/metric v1.38.0 // indirect | ||
| go.opentelemetry.io/otel/trace v1.38.0 // indirect | ||
| golang.org/x/net v0.46.0 // indirect | ||
| golang.org/x/sync v0.17.0 // indirect | ||
| golang.org/x/sys v0.37.0 // indirect | ||
| golang.org/x/text v0.30.0 // indirect | ||
| google.golang.org/genproto v0.0.0-20251103181224-f26f9409b101 // indirect | ||
| google.golang.org/genproto/googleapis/rpc v0.0.0-20251103181224-f26f9409b101 // indirect | ||
| google.golang.org/grpc v1.76.0 // indirect | ||
| google.golang.org/protobuf v1.36.10 // indirect | ||
| ) | ||
|
|
||
| replace github.com/opencontainers/runtime-spec => github.com/opencontainers/runtime-spec v1.2.1 // containerd compatibility |
There was a problem hiding this comment.
There's an inconsistency in the runtime-spec version. Line 67 declares runtime-spec v1.3.0 as a dependency, but line 91 replaces it with v1.2.1 for containerd compatibility. This is unusual - typically the replace directive should match the version in the require section, or the require section should directly specify v1.2.1. Consider updating line 67 to use v1.2.1 directly to avoid confusion.
| func (registry *Registry) GetImageDigests(ctx context.Context, repositoryName string, digest string) (digests []string, err error) { | ||
| manifest, err := registry.GetManifest(ctx, repositoryName, digest) | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| if manifest.MediaType == MediaTypeOCIIndexManifest { | ||
| err = ImageAlreadyIndexed | ||
| return | ||
| } | ||
|
|
||
| if manifest.MediaType == MediaTypeDockerManifestList { | ||
| // multi-arch iamge | ||
| for _, internalManifest := range manifest.Manifests { | ||
| if internalManifest.MediaType == MediaTypeDockerManifest { | ||
| internalDigest := fmt.Sprintf("%s:%s", internalManifest.Digest.Algorithm().String(), internalManifest.Digest.Encoded()) | ||
| if registry.ValidateImageManifest(ctx, repositoryName, internalDigest) == nil { | ||
| digests = append(digests, internalDigest) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if len(digests) == 0 { | ||
| err = fmt.Errorf("Manifest contains no valid images.") | ||
| } | ||
|
|
||
| return | ||
| } | ||
|
|
||
| if manifest.Config.MediaType == "" { | ||
| err = fmt.Errorf("Empty config media type.") | ||
| return | ||
| } | ||
|
|
||
| for _, configMediaType := range ImageConfigMediaTypes { | ||
| if manifest.Config.MediaType == configMediaType { | ||
| digests = append(digests, digest) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| err = fmt.Errorf("Unexpected config media type: %s, expected one of: %v.", manifest.Config.MediaType, ImageConfigMediaTypes) | ||
| return | ||
| } |
There was a problem hiding this comment.
The new GetImageDigests method lacks test coverage. Since this codebase has established testing practices (as seen in utils/registry/registry_test.go with tests for HeadManifest and GetManifest), similar test coverage should be added for this new method to ensure it correctly handles multi-arch images, single images, and already-indexed images.
mfittko
left a comment
There was a problem hiding this comment.
Blocking: go.sum still contains merge conflict markers, which breaks builds and go mod tooling. Additionally, OCI index manifests are treated as “already indexed,” which skips valid multi-arch images, and tags are applied inside the per-digest loop, so multi-arch runs overwrite tags and only the last digest remains addressable. Please fix these and add tests around GetImageDigests/tagging to lock in the intended behavior.
| return | ||
| } | ||
|
|
||
| if manifest.MediaType == MediaTypeOCIIndexManifest { |
There was a problem hiding this comment.
Treating MediaTypeOCIIndexManifest as "already indexed" will skip valid OCI index (multi-arch) images. Those are common now; they should be handled like the Docker manifest list by iterating manifests. If you really want to skip SOCI indexes, consider detecting SOCI-specific annotations instead of blanket-skipping all OCI indexes.
| } | ||
|
|
||
| log.Info(ctx, BuildAndPushSuccessMessage) | ||
| for _, newTag := range newTags { |
There was a problem hiding this comment.
In multi-arch flows digests can contain multiple entries, but this tags every index with the same newTags. Each tag can only point to one descriptor, so the last digest in the loop will overwrite earlier tags. Consider tagging once (e.g., on the manifest list) or using unique per-digest tags/annotations to avoid losing earlier indexes.
mfittko
left a comment
There was a problem hiding this comment.
Re-review after merge-conflict resolution: two blockers still remain.
GetImageDigeststreatsMediaTypeOCIIndexManifestas "already indexed" and returns an error. That skips valid OCI index (multi-arch) images; these should be handled like Docker manifest lists or you should detect SOCI-specific artifacts instead of blanket-skipping all OCI indexes.- Tagging happens inside the per-digest loop. For multi-arch, each tag is overwritten by the last digest, so only one index remains addressable by tag. Tag once or generate per-digest tags/annotations.
Non-blockers: the runtime-spec replace mismatch is confusing but not necessarily wrong; comment typos; and missing unit tests for Tag/GetImageDigests (recommended).
No description provided.