feat: enhance manifest fetch

[yuehaoliang]

Resolve #531

Signed-off-by: Haoliang Yue [email protected]

[codecov-commenter]

Codecov Report

Merging #541 (5287bf1) into main (b1415c0) will decrease coverage by 1.24%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main     #541      +/-   ##
==========================================
- Coverage   73.68%   72.44%   -1.25%     
==========================================
  Files          13       13              
  Lines         532      508      -24     
==========================================
- Hits          392      368      -24     
  Misses        112      112              
  Partials       28       28              

Impacted Files	Coverage Δ
cmd/oras/internal/option/cache.go	66.66% <66.66%> (ø)

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

[shizhMSFT]

Should we refactor it into the option package?

/cc @qweeah

[yuehaoliang]

Should we refactor it into the option package?

/cc @qweeah

Created a option Cache.

[qweeah]

This code block appears everywhere if --descriptor and --pretty and both needed, any idea to refactor it? It doesn't necessarily need to be done in this PR /cc @lizMSFT

[yuehaoliang]

We're discussing about this change offline.

[yuehaoliang]

I was trying to write a better test here, but I didn't figure out a good way unless exporting the cache.target struct.

want := struct {
	oras.ReadOnlyTarget
	cache content.Storage
}{
	mockTarget,
	ociStore,
}

Building a struct like above doesn't work as want and got are not equal.

[qweeah]

It's not doable even if cache.target struct is exported because target.cache is not exported. I think we can keep it this way.

[qweeah]

This if block can be merged with the if block at the end:

1. If !opts.OutputDescriptor || opt.outputPath != "" => fetch manifest content
2. If opts.OutputDescriptor => resolve and output to stdout

[yuehaoliang]

I couldn't really follow your idea here.

In the case of fetching manifest content, there still might be a need to output descriptor, eg. --output manifest.json --descriptor. Since oras.FetchBytes returns both the content and the descriptor, so if content has already been fetched, we don't need to call oras.Resolve again.

Current logic:

if opts.OutputDescriptor && opts.outputPath == "" {
	// oras.Resolve   fetch manifest descriptor only
} else {
	// oras.FetchBytes   fetch manifest descriptor content

	if opts.outputPath == "" || opts.outputPath == "-" {
		// output manifest content

		return
	}

	// save manifest content into the local file
}

if opts.OutputDescriptor {
	// output manifest descriptor
}

In the logic below, for --output manifest.json --descriptor, there's a redundant call of oras.Resolve.

if !opts.OutputDescriptor || opt.outputPath != "" {
	// oras.FetchBytes   fetch manifest descriptor content

	if opts.outputPath == "" || opts.outputPath == "-" {
		// output manifest content

		return
	}

	// save manifest content into the local file
} 

if opts.OutputDescriptor {
	// oras.Resolve   fetch manifest descriptor only

	// output manifest descriptor
}

[qweeah]

Since oras.FetchBytes returns both the content and the descriptor, so if content has already been fetched, we don't need to call oras.Resolve again

This can be avoided by checking if the desc is an empty struct.

[qweeah]

It's not doable even if cache.target struct is exported because target.cache is not exported. I think we can keep it this way.

[shizhMSFT]

LGTM