-
Notifications
You must be signed in to change notification settings - Fork 187
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
refactor: use handlers for oras blob push
command
#1618
base: main
Are you sure you want to change the base?
refactor: use handlers for oras blob push
command
#1618
Conversation
Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1618 +/- ##
==========================================
+ Coverage 84.33% 84.36% +0.02%
==========================================
Files 120 122 +2
Lines 5464 5532 +68
==========================================
+ Hits 4608 4667 +59
- Misses 608 614 +6
- Partials 248 251 +3 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>
Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>
Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>
Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>
Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>
Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>
Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>
Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>
oras blob push
commandoras blob push
command
Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
// BlobPushHandler handles descriptor output for blob push events. | ||
type BlobPushHandler interface { | ||
// OnBlobPushed is called after a blob is pushed. | ||
OnBlobPushed() error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by logic, it should be
OnBlobPushed() error | |
OnBlobPushed(desc ocispec.Descriptor) error |
return err | ||
} | ||
|
||
if err := metadataHandler.OnBlobPushed(opts.AnnotatedReference()); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AnnotatedReference()
itself is text output.
@@ -31,5 +31,11 @@ type ManifestIndexCreateHandler interface { | |||
OnContentCreated(content []byte) error | |||
} | |||
|
|||
// BlobPushHandler handles descriptor output for blob push events. | |||
type BlobPushHandler interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is --descriptor
considered as metadata output?
What this PR does / why we need it:
The
oras blob push
on the main branch has several problems.Printer.Println
for status and metadata output, instead of handlers.--descriptor
is used, status output is still shown but nothing should be printed except the descriptor.This PR fixes all the above issues and refactors the code to use status, metadata and content handlers.
Current display:
![image](https://private-user-images.githubusercontent.com/103478229/413738615-5309ade3-da45-483c-880b-097118653b20.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk5OTg3ODMsIm5iZiI6MTczOTk5ODQ4MywicGF0aCI6Ii8xMDM0NzgyMjkvNDEzNzM4NjE1LTUzMDlhZGUzLWRhNDUtNDgzYy04ODBiLTA5NzExODY1M2IyMC5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjE5JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxOVQyMDU0NDNaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1jOWQ5ZTRlYzA1ZmIyNWQ5MjYyMWQ1ZTUzYzY4YjdlODZmMjk2ODhiMmRmYTFhMTU1NmMwNDY0NTY4ZGYzYzMxJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.b3faNUJL-cpRy5a8_WtISo_KLWPnF7r_vZl6OuTV2bY)
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Part of #1542
Please check the following list: