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

feat: add compatibility mode for push and attach cmd #741

Merged
merged 21 commits into from
Jan 13, 2023

Conversation

qweeah
Copy link
Contributor

@qweeah qweeah commented Jan 10, 2023

Resolves #739

Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
@codecov-commenter
Copy link

codecov-commenter commented Jan 10, 2023

Codecov Report

Merging #741 (7d570b0) into main (9db2066) will decrease coverage by 3.33%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #741      +/-   ##
==========================================
- Coverage   73.46%   70.12%   -3.34%     
==========================================
  Files          17       18       +1     
  Lines         588      616      +28     
==========================================
  Hits          432      432              
- Misses        125      153      +28     
  Partials       31       31              
Impacted Files Coverage Δ
cmd/oras/internal/option/spec.go 0.00% <0.00%> (ø)

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

Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
@qweeah qweeah marked this pull request as ready for review January 11, 2023 12:52
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
cmd/oras/attach.go Outdated Show resolved Hide resolved
cmd/oras/attach.go Outdated Show resolved Hide resolved
cmd/oras/attach.go Outdated Show resolved Hide resolved
cmd/oras/internal/option/spec.go Outdated Show resolved Hide resolved
cmd/oras/internal/option/spec.go Outdated Show resolved Hide resolved
cmd/oras/internal/option/spec.go Outdated Show resolved Hide resolved
cmd/oras/internal/option/spec.go Outdated Show resolved Hide resolved
cmd/oras/internal/option/spec.go Outdated Show resolved Hide resolved
cmd/oras/push.go Outdated Show resolved Hide resolved
cmd/oras/push.go Outdated Show resolved Hide resolved
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
cmd/oras/internal/option/spec.go Outdated Show resolved Hide resolved
cmd/oras/internal/option/spec.go Outdated Show resolved Hide resolved
cmd/oras/internal/option/spec.go Outdated Show resolved Hide resolved
cmd/oras/push.go Outdated Show resolved Hide resolved
cmd/oras/push.go Outdated
Comment on lines 107 to 109
if opts.ManifestMediaType == ocispec.MediaTypeArtifactManifest && opts.manifestConfigRef != "" {
return errors.New("cannot pack an OCI artifact with manifest config at the same time")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It reminds me that there will be a feature request eventually to support --config for oras attach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? The content of config doesn't matter and the media type of config can be deducted from artifact type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they could, they will ask.

Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Signed-off-by: Billy Zha <jinzha1@microsoft.com>
This reverts commit 4b13256.

Signed-off-by: Billy Zha <jinzha1@microsoft.com>
@@ -51,6 +53,14 @@ func attachCmd() *cobra.Command {
Example - Attach file 'hi.txt' with type 'doc/example' to manifest 'hello:test' in registry 'localhost:5000'
oras attach --artifact-type doc/example localhost:5000/hello:test hi.txt

Example - Attach file "hi.txt" with specific manifest type for packing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest always splitting the sample commands into multiple examples. If you have more than two options under a flag and put all of those sample commands under a flag example, the layout looks too verbose.

Signed-off-by: Billy Zha <jinzha1@microsoft.com>
cmd/oras/push.go Outdated Show resolved Hide resolved
cmd/oras/push.go Outdated Show resolved Hide resolved
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@FeynmanZhou FeynmanZhou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shizhMSFT shizhMSFT merged commit 188df66 into oras-project:main Jan 13, 2023
@shizhMSFT shizhMSFT mentioned this pull request Jan 16, 2023
1 task
TerryHowe pushed a commit to TerryHowe/oras that referenced this pull request Feb 2, 2023
Resolves oras-project#739

Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants