-
Notifications
You must be signed in to change notification settings - Fork 177
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: status, metadata and content handlers for manifest index
commands
#1509
base: main
Are you sure you want to change the base?
Conversation
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>
manifest index
commands
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1509 +/- ##
==========================================
- Coverage 83.84% 82.90% -0.95%
==========================================
Files 118 119 +1
Lines 5156 5281 +125
==========================================
+ Hits 4323 4378 +55
- Misses 592 641 +49
- Partials 241 262 +21 ☔ View full report in Codecov by Sentry. |
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>
cmd/oras/internal/display/metadata/text/manifest_index_create.go
Outdated
Show resolved
Hide resolved
} | ||
|
||
// ManifestIndexUpdateHandler handles status output for manifest index update command. | ||
type ManifestIndexUpdateHandler 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.
Contains duplicated methods ManifestIndexCreateHandler
as below
OnIndexFetching(indexRef string) error
OnIndexFetched(indexRef string, digest digest.Digest) error
You can create a new interface and use it in both interfaces.
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.
Created an interface ManifestReferenceFetchHandler
and both index create and index update handlers contain it.
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.
Based on this comment, this interface is now removed.
// ignore --pretty when output to a file | ||
if opts.outputPath != "" && opts.outputPath != "-" { | ||
opts.Pretty.Pretty = false | ||
} |
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.
Can be merged into the handler construction.
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.
If this is a general option thing, maybe a method in otpions?
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.
If this is a general option thing, maybe a method in otpions?
@TerryHowe We can move it to content handler to avoid code repeat.
// OnContentCreated is called after index content is created. | ||
func (h *manifestIndexCreate) OnContentCreated(manifest []byte) error { | ||
out := h.stdout | ||
if h.outputPath != "-" && h.outputPath != "" { |
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.
if you changed outputPath to "" for "-" this logic would look less odd.
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.
Did you mean to put "" before "-"? Changed the if condition to if outputPath != "" && outputPath != "-"
} | ||
|
||
// NewManifestIndexUpdateHandler returns status, metadata and content handlers for index update command. | ||
func NewManifestIndexUpdateHandler(outputPath string, printer *output.Printer, pretty bool) ( |
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.
should there just be a NewManivestIndexHandler
method for both for now and someone can add the other in the future if needed?
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.
Currently we need both create and update handlers.
tmich := TextManifestIndexCreateHandler{ | ||
printer: printer, | ||
} | ||
return &tmich |
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.
The current naming style is aligned with the rest of the file.
miuh := TextManifestIndexUpdateHandler{ | ||
printer: printer, | ||
} | ||
return &miuh |
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.
The current naming style is aligned with the rest of the file.
@@ -188,7 +197,8 @@ func getPlatform(ctx context.Context, target oras.ReadOnlyTarget, manifestBytes | |||
return &platform, nil | |||
} | |||
|
|||
func pushIndex(ctx context.Context, target oras.Target, desc ocispec.Descriptor, content []byte, ref string, extraRefs []string, path string, printer *output.Printer) error { | |||
func pushIndex(ctx context.Context, onIndexPushed func(path string) error, onTagged func(desc ocispec.Descriptor, tag string) 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.
This is a private method here, seems like it would make more sense for it to take handlers than function pointers.
Another thing, it seems like an extremely long list of params like something is not right here.
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.
Since both create
and update
use this method and index create handlers and index update handlers have different types, the function argument can't take handler argument.
Yeah this is an extremely long list of parameters, but I can't find another way :(
// ignore --pretty when output to a file | ||
if opts.outputPath != "" && opts.outputPath != "-" { | ||
opts.Pretty.Pretty = false | ||
} |
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.
If this is a general option thing, maybe a method in otpions?
Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>
tmich := TextManifestIndexCreateHandler{ | ||
printer: printer, | ||
} | ||
return &tmich |
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.
The current naming style is aligned with the rest of the file.
miuh := TextManifestIndexUpdateHandler{ | ||
printer: printer, | ||
} | ||
return &miuh |
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.
The current naming style is aligned with the rest of the file.
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>
|
||
// ManifestIndexUpdateHandler handles text metadata output for index update events. | ||
type ManifestIndexUpdateHandler struct { | ||
printer *output.Printer |
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.
Can you help separate ManifestIndexUpdateHandler
and ManifestIndexCreateHandler
into manifest_index_update.go
and manifest_index_create.go
?
OnManifestRemoved(digest digest.Digest) error | ||
OnManifestAdded(manifestRef string, digest digest.Digest) error | ||
OnIndexMerged(indexRef string, digest digest.Digest) 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.
Change digest to descriptor since it will be used for formatted output in the future.
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.
The parameter of OnManifestRemoved(digest digest.Digest) error
must be a digest, since the descriptor is unknown. Changed the parameter of the other two functions as descriptors.
cmd/oras/internal/display/handler.go
Outdated
status.ManifestIndexCreateHandler, | ||
metadata.ManifestIndexCreateHandler, | ||
content.ManifestIndexCreateHandler, | ||
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.
When will an error be returned?
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.
good catch. Removed the error return.
@@ -52,12 +52,6 @@ const ( | |||
const ( | |||
IndexPromptFetching = "Fetching " |
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.
Extra space, same for fetched.
IndexPromptFetching = "Fetching " | |
IndexPromptFetching = "Fetching" |
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.
We are not doing status output indentation align anymore?
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.
Yes, there are still aligning work but there is trailing space for both Fetching
and Fetched
. Should remove one space from both prompts.
func pushIndex(ctx context.Context, onIndexPushed func(path string) error, onTagged func(desc ocispec.Descriptor, tag string) error, | ||
target oras.Target, desc ocispec.Descriptor, content []byte, ref string, extraRefs []string, path string) 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.
Try using handlers like:
func pushIndex(ctx context.Context, onIndexPushed func(path string) error, onTagged func(desc ocispec.Descriptor, tag string) error, | |
target oras.Target, desc ocispec.Descriptor, content []byte, ref string, extraRefs []string, path string) error { | |
func pushIndex(ctx context.Context, pmh metadata.ManifestPackHandler, th metadata.TaggedHandler,target oras.Target, desc ocispec.Descriptor, content []byte, ref string, extraRefs []string, path string) 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.
This function signature is now func pushIndex(ctx context.Context, displayStatus status.ManifestIndexCreateHandler, taggedHandler metadata.TaggedHandler, target oras.Target, desc ocispec.Descriptor, content []byte, ref string, extraRefs []string, path string)
after resolving this comment. Still long param list.
// NewDiscardHandler returns a new discard handler. | ||
func NewDiscardHandler() ManifestFetchHandler { | ||
func NewDiscardHandler() discardHandler { |
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.
Should we export discardHandler
just like status.DiscardHandler
?
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.
As NewDiscardHandler
is a public method, its returned type should be exported. Exported all discard
handlers in content, metadata and status packages.
if err != nil { | ||
return fmt.Errorf("failed to open %q: %w", h.outputPath, err) | ||
} | ||
defer f.Close() |
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.
We need to create an issue to track handling errors returned by Close()
in all write paths.
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.
Created #1516
// ignore --pretty when output to a file | ||
if outputPath != "" && outputPath != "-" { | ||
pretty = false | ||
} |
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.
It is different from NewManifestFetchHandler
. Is NewManifestFetchHandler
wrong? /cc @qweeah
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.
NewManifestFetchHandler
is used only once by oras manifest fetch
and such code snippet is in command code
oras/cmd/oras/root/manifest/fetch.go
Lines 80 to 83 in dcef719
// ignore --pretty when output to a file | |
case opts.outputPath != "" && opts.outputPath != "-": | |
opts.Pretty.Pretty = false | |
} |
I think we need to remove it and change the flag reset to NewManifestFetchHandler
too.
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.
Added pretty value reset in manifest fetch handler too.
type manifestPackHandler interface { | ||
OnIndexPacked(desc ocispec.Descriptor) error | ||
OnIndexPushed(path string) error | ||
OnCompleted(desc ocispec.Descriptor) 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.
Do you want to merge this to ManifestIndexCreateHandler
?
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.
Removed this interface and now moved these three methods to index create handler.
// referenceFetchHandler handles status output for reference fetch events. | ||
type referenceFetchHandler interface { | ||
OnFetching(manifestRef string) error | ||
OnFetched(manifestRef string, desc ocispec.Descriptor) 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.
Do you want to merge it to ManifestIndexCreateHandler
?
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.
Removed this interface and now moved these three methods to index create handler.
IndexPromptFetching = "Fetching " | ||
IndexPromptFetched = "Fetched " |
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.
Why those 2 are left?
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.
As per offline discussion, added back "added, removed, merged, pushed, packed"
Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>
4daf7d6
to
3a2fb99
Compare
As per offline discussion, added, merged, removed, pushed, packed should exist in both status and metadata output. And Status output is needed in the PR, metadata output can be added in a future pr when needed. |
Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>
Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>
What this PR does / why we need it:
Implements output handlers for
manifest index
commands.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #1503
Please check the following list: