Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Tracked by #369
SteveLasker opened this issue Mar 30, 2022 · 13 comments · Fixed by #468
Closed
Tracked by #369

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

SteveLasker opened this issue Mar 30, 2022 · 13 comments · Fixed by #468
Assignees
Labels
breaking change Public API changes enhancement New feature or request
Milestone

Comments

@SteveLasker
Copy link
Contributor

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
@sajayantony
Copy link
Contributor

+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

@shizhMSFT shizhMSFT added this to the v0.14.0 milestone May 7, 2022
@qweeah
Copy link
Contributor

qweeah commented Jul 26, 2022

We also need to add this for attach cmd.

@FeynmanZhou FeynmanZhou moved this to Todo in ORAS-Planning Jul 26, 2022
@FeynmanZhou
Copy link
Member

We also need to add this for attach cmd.

+1, I left more context in #407 (comment).

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?

@SteveLasker
Copy link
Contributor Author

SteveLasker commented Jul 26, 2022

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

@FeynmanZhou FeynmanZhou added the enhancement New feature or request label Jul 27, 2022
@junczhu
Copy link
Contributor

junczhu commented Jul 27, 2022

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

@junczhu
Copy link
Contributor

junczhu commented Aug 1, 2022

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

@FeynmanZhou
Copy link
Member

  • 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.

@shizhMSFT shizhMSFT moved this from Todo to In Progress in ORAS-Planning Aug 2, 2022
@sajayantony
Copy link
Contributor

sajayantony commented Aug 2, 2022

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.

@SteveLasker
Copy link
Contributor Author

SteveLasker commented Aug 2, 2022

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.

@sajayantony
Copy link
Contributor

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.

@FeynmanZhou
Copy link
Member

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" \

@qweeah
Copy link
Contributor

qweeah commented Aug 13, 2022

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?

Repository owner moved this from In Progress to Done in ORAS-Planning Aug 15, 2022
@FeynmanZhou
Copy link
Member

FeynmanZhou commented Aug 17, 2022

@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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Public API changes enhancement New feature or request
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants