option: support layer-level annotations via --annotation flag - #2083
option: support layer-level annotations via --annotation flag#2083BRGOVIND wants to merge 5 commits into
Conversation
oras push --annotation currently only supports manifest-level annotations. Users who need layer-level annotations must generate a temporary JSON file and pass it with --annotation-file, which is cumbersome in CI/CD pipelines. This change extends the --annotation flag to accept an optional file prefix: --annotation "key=value" (existing behavior, manifest-level) --annotation "filename.tar:key=value" (new, layer-level bound to filename) When a colon appears in the key part (before the first =), everything before the colon is treated as the file reference and everything after as the annotation key. Duplicate key detection is applied per-target. The resulting Annotations map mirrors the annotation-file JSON format, so no downstream changes are needed in the push path. Closes oras-project#2031 Signed-off-by: B R GOVIND <brgovind2005@gmail.com>
TerryHowe
left a comment
There was a problem hiding this comment.
Seems reasonable to me although I'm curious what other maintainers think.
It will need some tests though unit and ideally an e2e
Cover manifest-level, layer-level, mixed, duplicate key errors, missing '=' format error, empty key after file prefix, and values containing '=' characters. Signed-off-by: B R GOVIND <brgovind2005@gmail.com>
|
Thanks for the review @TerryHowe! I've pushed unit tests for the layer-level annotation parsing ( Happy to add an e2e case as well — since you mentioned wanting other maintainers' input on the approach, I held off on the heavier e2e suite until the design direction is confirmed. Just let me know and I'll add it. Re-requesting review when you get a chance. |
TerryHowe
left a comment
There was a problem hiding this comment.
Good base here, as well as the other comments, can you update the push and attach help text for this? Thanks!
Address review feedback on layer-level --annotation support: - Reject an empty target before the colon (e.g. ":key=value") so the intent is explicit, with guidance to use "key=value" or "target:key=value". - Treat the colon prefix as a generic target, allowing the special "$config" and "$manifest" scopes in addition to filenames. This also fixes a bug where "$manifest:key=value" overwrote bare "key=value" manifest annotations instead of merging, and now applies duplicate-key detection across both forms. - Validate that every layer (file) annotation target refers to a file in the command, so typos surface as an error instead of being silently dropped by loadFiles. Validation runs only for the --annotation flag path, which is mutually exclusive with --annotation-file, so existing annotation-file behavior is unchanged. - Update the --annotation flag help and add push/attach examples covering layer and config annotations. Extend unit tests for empty-target errors, the $config/$manifest targets, cross-form manifest duplicates, and file-target validation. Signed-off-by: B R GOVIND <brgovind2005@gmail.com>
| // | ||
| // Accepted formats: | ||
| // - "key=value" → manifest-level annotation | ||
| // - "target:key=value" → annotation scoped to an explicit target: |
There was a problem hiding this comment.
This feature would technically be a breaking change for keys that had a : in them previously it should be noted.
There was a problem hiding this comment.
This breaking change here is still an issue
There was a problem hiding this comment.
Any innovative ways to get around the breaking nature of this change?
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 30 days. |
|
Hi @BRGOVIND, |
that'd be great |
Signed-off-by: Jorge Morla <jorgemorlapro@gmail.com>
|
@BRGOVIND I created this Pull Request which is pointing to your PR to resolve @TerryHowe's comments. please let me know what you think |
test(e2e): cover targeted annotations for push and attach
Awesome, thanks Morla! I’ve merged the PR into my branch. appreciate you jumping in and handling the push updates and E2E coverage |
| // It only runs for the `--annotation` flag path; the `--annotation-file` path | ||
| // is mutually exclusive with it (see parseAnnotations) and is left untouched | ||
| // to preserve backward compatibility. | ||
| func (opts *Packer) validateAnnotationTargets() error { |
There was a problem hiding this comment.
This validate method, doesn't it seem like it is a natural fit in annotation.go maybe called in Parse?
There was a problem hiding this comment.
Good point.
I considered moving this into annotation.go/Parse, but this validation is specifically concerned with whether the parsed target is valid for the packer’s current context, rather than parsing the annotation syntax itself. Keeping it here also leaves the existing ones behavior unchanged for backward compatibility. I’m happy to move it if you think that separation would be clearer.
| // | ||
| // Accepted formats: | ||
| // - "key=value" → manifest-level annotation | ||
| // - "target:key=value" → annotation scoped to an explicit target: |
There was a problem hiding this comment.
Any innovative ways to get around the breaking nature of this change?
Closes #2031 — extends
--annotationto accept layer-level annotations directly, without needing a temporary--annotation-file.Problem
oras push --annotationcurrently only supports manifest-level annotations.To set layer-level annotations users must create a temporary JSON file and pass it with
--annotation-file:{ "": {"org.opencontainers.image.title": "demo"}, "myfile.tar": {"com.example.layer": "data"} }This is cumbersome in CI/CD pipelines and scripting.
Solution
Extend the
--annotationflag to accept an optional file prefix:--annotation "key=value"--annotation "filename.tar:key=value"When the key part (before
=) contains a colon, everything before the colon is treated as the file reference and everything after as the annotation key. OCI annotation keys use reverse-DNS with dots (not colons), so:) is safe as a separator.Duplicate key detection is applied per target (manifest or per-file). The resulting
Annotationsmap mirrors the--annotation-fileJSON format exactly, so no changes are needed downstream in the push path.Example