feat!: pass annotations as a string

[junczhu]

Resolve #362

BREAKING CHANGE: update annotation UX
Signed-off-by: Juncheng Zhu junczhu@microsoft.com

[codecov-commenter]

Codecov Report

Merging #468 (c0ea7d6) into main (7c6fbdb) will increase coverage by 11.88%.
The diff coverage is 81.25%.

@@             Coverage Diff             @@
##             main     #468       +/-   ##
===========================================
+ Coverage   55.69%   67.58%   +11.88%     
===========================================
  Files           6        7        +1     
  Lines         237      327       +90     
===========================================
+ Hits          132      221       +89     
+ Misses         90       85        -5     
- Partials       15       21        +6     

Impacted Files	Coverage Δ
cmd/oras/internal/option/packer.go	81.25% <81.25%> (ø)
internal/cache/target.go	72.46% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[qweeah]

We need unit tests. Let there be unit tests.

[junczhu]

Going to update UX, as discussed with the community, exiting with error when both flags are provided is a good option for now. @shizhMSFT

/cc @sajayantony @qweeah

[qweeah]

Going to update UX, as discussed with the community, exiting with error when both flags are provided is a good option for now. @shizhMSFT

cc. @sajayantony @qweeah

@junczhuMSFT This looks good. I think #466 can also apply this UX: if both manifest-config and artifact-type are specified for oras push, then exit with error /cc @yuehaoliang-microsoft

[qweeah]

@junczhuMSFT Since this changes the pusher option, we should also update CLI help doc for oras push and oras attach

[junczhu]

@junczhuMSFT Since this changes the pusher option, we should also update CLI help doc for oras push and oras attach

Well Noted

[junczhu]

We need unit tests. Let there be unit tests.

Working on it

[qweeah]

LGTM

[shizhMSFT]

LGTM

[vsoch]

Just a clarifying question here! In the first implementation I have, the user could provide an annotations file, and this wasn't just for the manifest, you could have keys $manifest or $config or the fullpath/relative path of a blob to indicate that we want to add annotations there. But now the new argument --annotation key=value seems to be scoped just for the final manifest, and not a config or particular blob. Is this a correct interpretation?

[vsoch]

And in this case, wouldn't we only care if both are defined given we have $manifest in the file lookup plus an --annotation? E.g., providing this new flag with an annotation file without manifest annotations should not be an issue, but as I see here the client is just disallowing both to be defined.

[junczhu]

Hello @vsoch thank you for insight.

1. If users want to update annotations including keys $manifest, $config and more, they can use --annotation-file and provide an annotations file
2. If users only want to update manifest annotations, they can use --annotation. They don't have to create a file just to upload a single annotation
3. I have set up a condition that --annotation and --annotation-file cannot be both specified to make sure their implements are isolated to each other

[vsoch]

Yes so my question is why not to allow a file with $manifest and a one off annotation that has a key that doesn’t overlap? It could be a use case given that someone has a file with common/shared annotations and then a few custom to a container. I think this should be allowed because there is no complexity in terms of duplicate keys - it’s just reading a file and then adding one off annotations from the command line. And you’d immediately reject/exit given duplicates only.