refactor: status, metadata and content handlers for `manifest index` commands #1509

wangxiaoxuan273 · 2024-09-24T08:55:43Z

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:

Does the affected code have corresponding tests, e.g. unit test, E2E test?
Does this change require a documentation update?
Does this introduce breaking changes that would require an announcement or bumping the major version?
Do all new files have an appropriate license header?

Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>

codecov · 2024-09-24T08:58:28Z

Codecov Report

Attention: Patch coverage is 73.46939% with 52 lines in your changes missing coverage. Please review.

Project coverage is 85.28%. Comparing base (8dc05a7) to head (4d18845).

Files with missing lines	Patch %	Lines
cmd/oras/root/manifest/index/update.go	47.82%	12 Missing and 12 partials ⚠️
cmd/oras/root/manifest/index/create.go	44.00%	7 Missing and 7 partials ⚠️
cmd/oras/internal/display/status/discard.go	75.00%	6 Missing ⚠️
cmd/oras/internal/display/status/text.go	90.47%	3 Missing and 1 partial ⚠️
...md/oras/internal/display/content/manifest_index.go	85.71%	1 Missing and 1 partial ⚠️
cmd/oras/internal/display/metadata/discard.go	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1509      +/-   ##
==========================================
- Coverage   86.10%   85.28%   -0.83%     
==========================================
  Files         118      119       +1     
  Lines        4224     4355     +131     
==========================================
+ Hits         3637     3714      +77     
- Misses        350      382      +32     
- Partials      237      259      +22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>

cmd/oras/internal/display/content/manifest_index.go

Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>

qweeah · 2024-09-26T03:26:13Z

cmd/oras/root/manifest/index/create.go

@@ -91,6 +89,10 @@ Example - Create an index and output the index to stdout, auto push will be disa
 			opts.RawReference = refs[0]
 			opts.extraRefs = refs[1:]
 			opts.sources = args[1:]
+			// ignore --pretty when output to a file
+			if opts.outputPath != "" && opts.outputPath != "-" {
+				opts.Pretty.Pretty = false


Should move this pretty reset within OnContentCreated, so you don't need to duplicate this snippet in index update

In the option handling in command maybe?

qweeah · 2024-09-26T03:26:54Z

cmd/oras/internal/display/content/manifest_index.go

+
+// NewManifestIndexCreateHandler creates a new handler.
+func NewManifestIndexCreateHandler(out io.Writer, pretty bool, outputPath string) ManifestIndexCreateHandler {
+	return &manifestIndexCreate{


reset pretty here if output path is -

qweeah · 2024-09-26T03:46:16Z

cmd/oras/internal/display/metadata/text/manifest_index_create.go

@@ -37,3 +38,8 @@ func NewManifestIndexCreateHandler(printer *output.Printer) metadata.ManifestInd
 func (h *ManifestIndexCreateHandler) OnTagged(_ ocispec.Descriptor, tag string) error {
 	return h.printer.Println("Tagged", tag)
 }
+
+// OnCompleted implements ManifestIndexCreateHandler.
+func (h *ManifestIndexCreateHandler) OnCompleted(digest digest.Digest) error {


Considering we are most likely to have format. Here should pass in a descriptor and output the digest of the descriptor

qweeah · 2024-09-26T03:46:52Z

cmd/oras/internal/display/metadata/discard.go

@@ -28,3 +31,13 @@ func NewDiscardHandler() discard {
 func (discard) OnFetched(string, ocispec.Descriptor, []byte) error {
 	return nil
 }
+
+// OnTagged implements ManifestIndexCreateHandler.
+func (discard) OnTagged(desc ocispec.Descriptor, tag string) error {


NIT

Suggested change

func (discard) OnTagged(desc ocispec.Descriptor, tag string) error {

func (discard) OnTagged(ocispec.Descriptor, string) error {

qweeah · 2024-09-26T03:47:09Z

cmd/oras/internal/display/metadata/discard.go

+}
+
+// OnCompleted implements ManifestIndexCreateHandler.
+func (discard) OnCompleted(digest digest.Digest) error {


Suggested change

func (discard) OnCompleted(digest digest.Digest) error {

func (discard) OnCompleted(ocispec.Descriptor) error {

qweeah · 2024-09-26T03:47:47Z

cmd/oras/internal/display/metadata/interface.go

@@ -81,8 +82,12 @@ type ManifestPushHandler interface {
 // ManifestIndexCreateHandler handles metadata output for index create events.
 type ManifestIndexCreateHandler interface {
 	TaggedHandler
+	OnCompleted(digest digest.Digest) error


Suggested change

OnCompleted(digest digest.Digest) error

OnCompleted(ocispec.Descriptor) error

qweeah · 2024-09-26T04:45:15Z

cmd/oras/internal/display/status/interface.go

+type ManifestIndexCreateHandler interface {
+	OnSourceManifestFetching(source string) error
+	OnSourceManifestFetched(source string) error
+	OnIndexPacked(shortDigest string) error


Packed index should be moved to metadata output.

Packed, Updated, Pushed, Added, Removed, Merged should all be moved to metadata output.

Use one interface for packed and updated.

qweeah · 2024-09-26T04:46:37Z

cmd/oras/internal/display/status/interface.go

+	OnSourceManifestFetching(source string) error
+	OnSourceManifestFetched(source string) error


Can we remove Source from the naming? Will non-source manifest be fetched?

qweeah · 2024-09-26T04:47:00Z

cmd/oras/internal/display/status/interface.go

+	OnManifestRemoved(digest digest.Digest) error
+	OnManifestAdded(manifestRef string, digest digest.Digest) error
+	OnIndexMerged(indexRef string, digest digest.Digest) error


Should be moved to metadata handler

qweeah · 2024-09-26T04:58:32Z

cmd/oras/internal/display/status/interface.go

+}
+
+// ManifestIndexUpdateHandler handles status output for manifest index update command.
+type ManifestIndexUpdateHandler interface {


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.

qweeah · 2024-09-26T06:17:58Z

cmd/oras/internal/display/handler.go

+	var statusHandler status.ManifestIndexCreateHandler
+	var metadataHandler metadata.ManifestIndexCreateHandler
+	var contentHandler content.ManifestIndexCreateHandler
+	switch outputPath {
+	case "":
+		statusHandler = status.NewTextManifestIndexCreateHandler(printer)
+		metadataHandler = text.NewManifestIndexCreateHandler(printer)
+		contentHandler = content.NewDiscardHandler()
+	case "-":
+		statusHandler = status.NewDiscardHandler()
+		metadataHandler = metadata.NewDiscardHandler()
+		contentHandler = content.NewManifestIndexCreateHandler(printer, pretty, outputPath)
+	default:
+		statusHandler = status.NewTextManifestIndexCreateHandler(printer)
+		metadataHandler = text.NewManifestIndexCreateHandler(printer)
+		contentHandler = content.NewManifestIndexCreateHandler(printer, pretty, outputPath)
+	}


Suggested change

var statusHandler status.ManifestIndexCreateHandler

var metadataHandler metadata.ManifestIndexCreateHandler

var contentHandler content.ManifestIndexCreateHandler

switch outputPath {

case "":

statusHandler = status.NewTextManifestIndexCreateHandler(printer)

metadataHandler = text.NewManifestIndexCreateHandler(printer)

contentHandler = content.NewDiscardHandler()

case "-":

statusHandler = status.NewDiscardHandler()

metadataHandler = metadata.NewDiscardHandler()

contentHandler = content.NewManifestIndexCreateHandler(printer, pretty, outputPath)

default:

statusHandler = status.NewTextManifestIndexCreateHandler(printer)

metadataHandler = text.NewManifestIndexCreateHandler(printer)

contentHandler = content.NewManifestIndexCreateHandler(printer, pretty, outputPath)

}

statusHandler := status.NewTextManifestIndexCreateHandler(printer)

metadataHandler := text.NewManifestIndexCreateHandler(printer)

contentHandler := content.NewManifestIndexCreateHandler(printer, pretty, outputPath)

switch outputPath {

case "":

contentHandler = content.NewDiscardHandler()

case "-":

statusHandler = status.NewDiscardHandler()

metadataHandler = metadata.NewDiscardHandler()

}

qweeah · 2024-09-26T06:20:45Z

cmd/oras/root/manifest/index/update.go

+			// ignore --pretty when output to a file
+			if opts.outputPath != "" && opts.outputPath != "-" {
+				opts.Pretty.Pretty = false
+			}


Can be merged into the handler construction.

If this is a general option thing, maybe a method in otpions?

TerryHowe · 2024-09-27T17:50:46Z

cmd/oras/internal/display/content/manifest_index.go

+// OnContentCreated is called after index content is created.
+func (h *manifestIndexCreate) OnContentCreated(manifest []byte) error {
+	out := h.stdout
+	if h.outputPath != "-" && h.outputPath != "" {


if you changed outputPath to "" for "-" this logic would look less odd.

TerryHowe · 2024-09-27T17:52:08Z

cmd/oras/internal/display/handler.go

+}
+
+// NewManifestIndexUpdateHandler returns status, metadata and content handlers for index update command.
+func NewManifestIndexUpdateHandler(outputPath string, printer *output.Printer, pretty bool) (


should there just be a NewManivestIndexHandler method for both for now and someone can add the other in the future if needed?

TerryHowe · 2024-09-27T17:55:12Z

cmd/oras/internal/display/status/text.go

+	tmich := TextManifestIndexCreateHandler{
+		printer: printer,
+	}
+	return &tmich


Suggested change

tmich := TextManifestIndexCreateHandler{

printer: printer,

}

return &tmich

manifestHandler := TextManifestIndexCreateHandler{

printer: printer,

}

return &manifestHandler

TerryHowe · 2024-09-27T18:05:29Z

cmd/oras/internal/display/status/text.go

+	miuh := TextManifestIndexUpdateHandler{
+		printer: printer,
+	}
+	return &miuh


Suggested change

miuh := TextManifestIndexUpdateHandler{

printer: printer,

}

return &miuh

manifestHandler := TextManifestIndexUpdateHandler{

printer: printer,

}

return &manifestHandler

TerryHowe · 2024-09-27T18:06:35Z

cmd/oras/root/manifest/index/create.go

@@ -91,6 +89,10 @@ Example - Create an index and output the index to stdout, auto push will be disa
 			opts.RawReference = refs[0]
 			opts.extraRefs = refs[1:]
 			opts.sources = args[1:]
+			// ignore --pretty when output to a file
+			if opts.outputPath != "" && opts.outputPath != "-" {
+				opts.Pretty.Pretty = false


In the option handling in command maybe?

TerryHowe · 2024-09-27T18:11:21Z

cmd/oras/root/manifest/index/create.go

@@ -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,


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.

TerryHowe · 2024-09-27T18:13:44Z

cmd/oras/root/manifest/index/update.go

+			// ignore --pretty when output to a file
+			if opts.outputPath != "" && opts.outputPath != "-" {
+				opts.Pretty.Pretty = false
+			}


If this is a general option thing, maybe a method in otpions?

Xiaoxuan Wang added 5 commits September 24, 2024 10:39

create handler

182c8f0

Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>

compiles

495ab7e

Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>

align update output with tag

d81fd04

Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>

align update output with digest

f807af8

Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>

refinement

9134a8f

Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>

wangxiaoxuan273 changed the title ~~Output output~~ refactor: status, metadata and content handlers for manifest index commands Sep 24, 2024

Xiaoxuan Wang added 3 commits September 24, 2024 17:04

added error checks

5104cc9

Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>

content handlers

a9b1164

Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>

refinement

4d144be

Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>

wangxiaoxuan273 marked this pull request as ready for review September 25, 2024 05:57

wangxiaoxuan273 requested review from sajayantony, shizhMSFT, SteveLasker, qweeah and TerryHowe as code owners September 25, 2024 05:57

qweeah reviewed Sep 25, 2024

View reviewed changes

cmd/oras/internal/display/content/manifest_index.go Show resolved Hide resolved

fix pretty with file output

4d18845

Signed-off-by: Xiaoxuan Wang <xiaoxuanwang@microsoft.com>

qweeah requested changes Sep 26, 2024

View reviewed changes

qweeah reviewed Sep 26, 2024

View reviewed changes

qweeah requested changes Sep 26, 2024

View reviewed changes

TerryHowe reviewed Sep 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: status, metadata and content handlers for `manifest index` commands #1509

refactor: status, metadata and content handlers for `manifest index` commands #1509

wangxiaoxuan273 commented Sep 24, 2024

codecov bot commented Sep 24, 2024 •

edited

Loading

qweeah Sep 26, 2024

TerryHowe Sep 27, 2024

qweeah Sep 26, 2024

qweeah Sep 26, 2024

qweeah Sep 26, 2024

qweeah Sep 26, 2024

qweeah Sep 26, 2024

qweeah Sep 26, 2024

wangxiaoxuan273 Sep 26, 2024

wangxiaoxuan273 Sep 26, 2024

qweeah Sep 26, 2024

qweeah Sep 26, 2024

qweeah Sep 26, 2024

qweeah Sep 26, 2024

qweeah Sep 26, 2024

TerryHowe Sep 27, 2024

TerryHowe Sep 27, 2024

TerryHowe Sep 27, 2024

TerryHowe Sep 27, 2024

TerryHowe Sep 27, 2024

TerryHowe Sep 27, 2024

TerryHowe Sep 27, 2024

TerryHowe Sep 27, 2024

	func (discard) OnTagged(desc ocispec.Descriptor, tag string) error {
	func (discard) OnTagged(ocispec.Descriptor, string) error {

	func (discard) OnCompleted(digest digest.Digest) error {
	func (discard) OnCompleted(ocispec.Descriptor) error {

	OnCompleted(digest digest.Digest) error
	OnCompleted(ocispec.Descriptor) error

		OnSourceManifestFetching(source string) error
		OnSourceManifestFetched(source string) error

refactor: status, metadata and content handlers for manifest index commands #1509

Are you sure you want to change the base?

refactor: status, metadata and content handlers for manifest index commands #1509

Conversation

wangxiaoxuan273 commented Sep 24, 2024

codecov bot commented Sep 24, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refactor: status, metadata and content handlers for `manifest index` commands #1509

refactor: status, metadata and content handlers for `manifest index` commands #1509

codecov bot commented Sep 24, 2024 •

edited

Loading