oras icon indicating copy to clipboard operation
oras copied to clipboard

ORAS Push should support annotations, without having to specify a file

Open SteveLasker opened this issue 2 years ago • 10 comments

Repro steps

oras push $REGISTRY/$REPO \
    --artifact-type 'signature/example' \
    --subject $IMAGE \
    --manifest-annotations io.cncf.oras.artifact.created=$CREATED_DATE \
    ./readme.md:application/md

Fails, as it expects --manifest-annotations to be a file. I was hoping the docs were wrong :(

Flags:
      --artifact-type string          artifact type
  -c, --config stringArray            auth config path
  -d, --debug                         debug mode
      --disable-path-validation       skip path validation
      --dry-run                       push to a dummy registry instead of the actual remote registry
      --export-manifest string        export the pushed manifest
  -h, --help                          help for push
      --insecure                      allow connections to SSL registry without certs
      --manifest-annotations string   manifest annotation file

Desired

The default experience should enable a user to pass annotations as a string, so they don't have to create a file just to upload a single annotation I'd suggest a file could be sent in as a stream if we really needed to support files. We should also support multiple --manifest-annotation values to be passed in:

oras push $REGISTRY/$REPO \
    --artifact-type 'signature/example' \
    --subject $IMAGE \
    --manifest-annotations io.cncf.oras.artifact.created=$CREATED_DATE \
    --manifest-annotations io.cncf.oras.artifact.description="foo"\
    ./readme.md:application/md

SteveLasker avatar Mar 30 '22 22:03 SteveLasker

+1. I ran into this one as well. I think we need to target a v2 release of oras since this is breaking.

Recommend renaming these to singular with multi value support and not plural as called out in the issue.

—manifest-annotation maybe alias to -n and —manifest-annotations-file

sajayantony avatar Mar 31 '22 16:03 sajayantony

We also need to add this for attach cmd.

qweeah avatar Jul 26 '22 00:07 qweeah

We also need to add this for attach cmd.

+1, I left more context in https://github.com/oras-project/oras/discussions/407#discussioncomment-3232163.

As we decide to implement and support pass annotations without specifying a file for both oras push and oras attach in one PR, do we need to include oras attach in this issue title?

FeynmanZhou avatar Jul 26 '22 15:07 FeynmanZhou

+1 on splitting the annotations as strings and attach User may want to specific annotuonts on non-reference type artifacts

SteveLasker avatar Jul 26 '22 15:07 SteveLasker

Hi team, I am highly interested in the issue above, please assign to me @shizhMSFT

junczhu avatar Jul 27 '22 05:07 junczhu

  • [x] Keep annotation file by renaming the --manifest-annotations into --manifest-annotations-file
  • [x] Support manifest annotation flags via --annotations

junczhu avatar Aug 01 '22 10:08 junczhu

  • [ ] Rename the --manifest-annotations into --manifest-annotations-file
  • [ ] Use --manifest-annotations to add annotation into manifest

@junczhuMSFT The implementation plans look good to me. We can support these two ways in v0.14.0 and then deprecate the previous one going forward.

FeynmanZhou avatar Aug 02 '22 01:08 FeynmanZhou

Consider it you can just rename to annotations. Layer and config annotations are more coMplex scenario and so maybe if you wanted those we could use the —layer or —config prefix.

I would recommend keeping the default flag short and -a or something simpler as prefix.

sajayantony avatar Aug 02 '22 04:08 sajayantony

Thanks @FeynmanZhou Just to clarify, will the parameter be --manifest-annotation with or without the (s)? Would the CLI be:

oras attach acmerocket.azurecr.io/net-monitor:v1 \
  --annotation "foo:bar" \
  --annotation "something:true"

I've updated to use --annotation instead of --manifest-annotation per sajay's suggestion. I'm also not sure we need layer or config annotations in the oras cli. In the oras-go library, sure. Just not sure it's a common scenario to over-populate the cli at this point.

SteveLasker avatar Aug 02 '22 04:08 SteveLasker

I don’t think we need the config or layer in the CLI. Just discussing that it’s possible if and when the requirement comes, we have possibilities of opening up without affecting the —annotation flag.

sajayantony avatar Aug 02 '22 08:08 sajayantony

Thanks @FeynmanZhou Just to clarify, will the parameter be --manifest-annotation with or without the (s)? Would the CLI be:

oras attach acmerocket.azurecr.io/net-monitor:v1 \
  --annotation "foo:bar" \
  --annotation "something:true"

I've updated to use --annotation instead of --manifest-annotation per sajay's suggestion. I'm also not sure we need layer or config annotations in the oras cli. In the oras-go library, sure. Just not sure it's a common scenario to over-populate the cli at this point.

@SteveLasker You are correct. --annotation is the final UX. See the following examples:

ORAS Attach

oras attach acmerocket.azurecr.io/net-monitor:v1 \
  --annotation "key1:value1" \
  --annotation "key2:value2"

ORAS Push

oras push localhost:5000/hello:latest \
  --annotaion "key=value" \

FeynmanZhou avatar Aug 12 '22 17:08 FeynmanZhou

Thanks @FeynmanZhou Just to clarify, will the parameter be --manifest-annotation with or without the (s)? Would the CLI be:

oras attach acmerocket.azurecr.io/net-monitor:v1 \
  --annotation "foo:bar" \
  --annotation "something:true"

I've updated to use --annotation instead of --manifest-annotation per sajay's suggestion. I'm also not sure we need layer or config annotations in the oras cli. In the oras-go library, sure. Just not sure it's a common scenario to over-populate the cli at this point.

@SteveLasker You are correct. --annotation is the final UX. See the following examples:

ORAS Attach

oras attach acmerocket.azurecr.io/net-monitor:v1 \
  --annotation "key1:value1" \
  --annotation "key2:value2"

ORAS Push

oras push localhost:5000/hello:latest \
  --annotaion "key=value" \

@FeynmanZhou Why attach uses : but push uses = for delimiterring the annotation key and value? Is it a typo?

qweeah avatar Aug 13 '22 11:08 qweeah

@qweeah I think both should work but --annotation "key : value" is much standard according to the OCI annotation rules.. We can use : by default. BTW, regclient supports using --annotation "key = value".

FeynmanZhou avatar Aug 17 '22 15:08 FeynmanZhou