feat(xpkg): add --annotation flag to xpkg build and xpkg push#11
feat(xpkg): add --annotation flag to xpkg build and xpkg push#11chaitanyapantheor wants to merge 7 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughParses repeatable ChangesOCI Annotation CLI Support
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Parser as parseAnnotations
participant PushCmd as pushCmd.Run
participant MutateImage as annotateImage
participant MutateIndex as annotateIndex
participant Remote as remote.WriteIndex
CLI->>Parser: provide KEY=VALUE flags
Parser->>PushCmd: map[string]string annotations
PushCmd->>MutateImage: annotateImage(img, annotations)
PushCmd->>MutateIndex: annotateIndex(index, annotations)
MutateIndex->>Remote: remote.WriteIndex(annotated index)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Thanks — do you want a unit test added for annotateImage/annotateIndex behavior beyond parse tests? Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
4476efa to
a8a515b
Compare
adamwg
left a comment
There was a problem hiding this comment.
I've left a couple of thoughts, but overall this looks good to me and seems like a valuable feature.
| Package string `arg:"" help:"Where to push the package. Must be a fully qualified OCI tag, including the registry, repository, and tag." placeholder:"REGISTRY/REPOSITORY:TAG"` | ||
|
|
||
| // Flags. Keep sorted alphabetically. | ||
| Annotation []string `help:"An OCI manifest annotation to add to the package in key=value format. Repeatable." placeholder:"KEY=VALUE" short:"a"` |
There was a problem hiding this comment.
I wonder if this should be OCIAnnotation / --oci-annotation or something like that to make it clear how it's different from the metadata.annotations.
There was a problem hiding this comment.
Good point — renamed to --oci-annotation (Go field OCIAnnotation, explicit name:"oci-annotation" kong tag) in both xpkg build and xpkg push. Short flag -a is unchanged.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/crossplane/xpkg/annotations.go (1)
30-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider validating that annotation keys are non-empty.
The current implementation would accept an annotation like
=value(empty key), which could cause confusing errors later when pushing to the registry. Adding a simple validation would provide users with a clearer, earlier error message.🛡️ Suggested validation
func parseAnnotations(kvs []string) (map[string]string, error) { anns := make(map[string]string, len(kvs)) for _, kv := range kvs { k, v, ok := strings.Cut(kv, "=") if !ok { return nil, errors.Errorf("invalid annotation %q: must be in key=value format", kv) } + if k == "" { + return nil, errors.Errorf("invalid annotation %q: key cannot be empty", kv) + } anns[k] = v } return anns, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/xpkg/annotations.go` around lines 30 - 40, The parseAnnotations function accepts "k=v" pairs but doesn't reject empty keys (e.g., "=value"); update parseAnnotations to validate that the extracted key (variable k from strings.Cut on each kv) is non-empty and, if empty, return a clear formatted error (similar to the existing errors.Errorf) indicating the annotation has an empty key; ensure this check happens after the strings.Cut ok check and before assigning into anns so no empty-key entries are inserted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cmd/crossplane/xpkg/annotations.go`:
- Around line 30-40: The parseAnnotations function accepts "k=v" pairs but
doesn't reject empty keys (e.g., "=value"); update parseAnnotations to validate
that the extracted key (variable k from strings.Cut on each kv) is non-empty
and, if empty, return a clear formatted error (similar to the existing
errors.Errorf) indicating the annotation has an empty key; ensure this check
happens after the strings.Cut ok check and before assigning into anns so no
empty-key entries are inserted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0477f7aa-6ce3-4b3c-953a-d9cd84c3a305
📒 Files selected for processing (4)
cmd/crossplane/xpkg/annotations.gocmd/crossplane/xpkg/annotations_test.gocmd/crossplane/xpkg/build.gocmd/crossplane/xpkg/push.go
✅ Files skipped from review due to trivial changes (1)
- cmd/crossplane/xpkg/annotations_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/crossplane/xpkg/build.go
- cmd/crossplane/xpkg/push.go
75da5d2 to
f40a61f
Compare
adamwg
left a comment
There was a problem hiding this comment.
Thanks for the updates! Looks good to me, aside from the comment I've left on build - sorry for not calling it out earlier.
| Ignore []string `help:"comma-separated list of globs specifying files to exclude from the build, relative to --package-root." placeholder:"PATH"` | ||
| PackageFile string `help:"The file to write the package to. Defaults to a generated filename in --package-root." placeholder:"PATH" predictor:"xpkg_file" short:"o" type:"path"` | ||
| PackageRoot string `default:"." help:"The directory that contains the package's crossplane.yaml file." predictor:"directory" short:"f" type:"existingdir"` | ||
| OCIAnnotation []string `help:"An OCI manifest annotation to add to the package in key=value format. Repeatable." name:"oci-annotation" placeholder:"KEY=VALUE" short:"a"` |
There was a problem hiding this comment.
As far as I can tell, the image annotations actually get persisted anywhere by crossplane xpkg build (they're not represented anywhere in the tarball written by tarball.Write). Assuming that's true, we should remove the flag here and keep it on push only; otherwise, a user could reasonably expect that they don't need to provide annotations to push if they've already provided them to build, which isn't true.
There was a problem hiding this comment.
Good catch — you're right that annotations applied during build aren't stored in the tarball, so the flag was giving a false impression. I've removed --oci-annotation from xpkg build entirely; it now lives only on xpkg push where it can actually take effect.
Signed-off-by: Chaitanya Maili <chaitanya.maili@pantheon.io>
Signed-off-by: Chaitanya Maili <chaitanya.maili@pantheon.io>
Signed-off-by: Chaitanya Maili <chaitanya.maili@pantheon.io>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Chaitanya Maili <chaitanya.maili@pantheon.io>
- Rename --annotation to --oci-annotation in xpkg build and push to distinguish from Kubernetes metadata.annotations - Apply OCI annotations to the image index in the multi-platform push path, not only to individual manifests - Fix copyright year in annotations.go and annotations_test.go Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Chaitanya Maili <chaitanya.maili@pantheon.io>
Annotations like "=value" passed strings.Cut with ok=true, silently inserting an empty string key into the map. Add an explicit check and return a clear error when the key is empty. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Chaitanya Maili <chaitanya.maili@pantheon.io>
Annotations applied during build are not persisted in the tarball written by tarball.Write, so keeping the flag on build could mislead users into thinking they don't need to provide annotations to push. Move the flag and errParseAnnotations to push only. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Chaitanya Maili <chaitanya.maili@pantheon.io>
a485bcb to
22e4bff
Compare
adamwg
left a comment
There was a problem hiding this comment.
LGTM, thanks for the contribution!
Description of your changes
Adds a repeatable
--annotation/-a KEY=VALUEflag tocrossplane xpkg buildand
crossplane xpkg push, allowing users to attach OCI manifest annotationsto packages at build or push time.
Annotations are applied to the OCI image manifest. Malformed annotations
(missing =) return an error before any write or push occurs. Reading
annotations from crossplane.yaml is intentionally out of scope to avoid
silently propagating internal Crossplane annotations to the OCI registry.
Covered by unit tests in cmd/crossplane/xpkg/annotations_test.go.
Fixes crossplane/crossplane#7282
I have:
./nix.sh flake checkto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR.