Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Make errSkipDesc a publicly available error #487

Closed
wants to merge 2 commits into from

Conversation

Noxsios
Copy link

@Noxsios Noxsios commented Apr 19, 2023

With this change, oras.ErrSkipDesc can be used during PreCopy to do custom filtering during copy operations.

This error is needed to be public as errors.New will always create a new error instance, even if the text is the same, so you cannot utilize errors.New("skip descriptor").

ex (psuedo code):

        only := []string{"hello.txt", "world.txt"}
	copyOpts.PreCopy = func(ctx context.Context, desc ocispec.Descriptor) error {
		if len(only) > 0 {
			if !utils.sliceContains(only, desc.Annotations[ocispec.AnnotationTitle]) {
				return oras.ErrSkipDesc
			}
		}
		return nil
	}

Signed-off-by: razzle <harry@razzle.cloud>
Copy link
Member

@TerryHowe TerryHowe left a comment

Choose a reason for hiding this comment

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

Is there any other data that should be in this error like #482

@Noxsios
Copy link
Author

Noxsios commented Apr 19, 2023

Is there any other data that should be in this error like #482

AFAIK, no.

@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2023

Codecov Report

Merging #487 (925ef9e) into main (264f5f2) will increase coverage by 0.22%.
The diff coverage is 0.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #487      +/-   ##
==========================================
+ Coverage   72.92%   73.14%   +0.22%     
==========================================
  Files          49       49              
  Lines        4528     4528              
==========================================
+ Hits         3302     3312      +10     
+ Misses        919      911       -8     
+ Partials      307      305       -2     
Impacted Files Coverage Δ
copy.go 62.86% <0.00%> (ø)

... and 1 file with indirect coverage changes

@qweeah
Copy link
Contributor

qweeah commented Apr 24, 2023

@Noxsios IMHO, it's better to do this work in FindSuccessors?

only := []string{"hello.txt", "world.txt"}
copyOpts.FindSuccessors = func(ctx context.Context, fetcher content.Fetcher, desc ocispec.Descriptor) ([]ocispec.Descriptor, error) {
  nodes, err := func Successors(ctx, fetcher, desc)
  if err != nil {
    return nil, err
  }
  if len(only) == 0 {
    return nodes
  }
  var ret []ocispec.Descriptor
  for _, n := range nodes {
    if utils.sliceContains(only, n.Annotations[ocispec.AnnotationTitle]) /* or it's a non-leaf node like manifest/index */ {
      append(ret, n)     
    }
  }
  return ret
})

@Wwwsylvia
Copy link
Member

Hi @Noxsios, can you check if @qweeah's suggestion works for you?
My concern is that ErrSkipDesc might be easily misused by users.

@Noxsios
Copy link
Author

Noxsios commented May 5, 2023

@Wwwsylvia Agreed. I was unaware that FindSuccessors could mutate the fetched manifest before layers were pulled. I will test and close if it works as intended.

@Noxsios
Copy link
Author

Noxsios commented May 9, 2023

Looks like that works! Final function looks similar to this in case others want the same behavior.

// where optional is a slice of strings
// estimatedBytes is the result of calculating the total size of a OCI artifact
		alwaysPull := []string{"zarf.yaml", "checksums.txt", "zarf.yaml.sig"}
		toPull = append(optional, alwaysPull...)
		copyOpts.FindSuccessors = func(ctx context.Context, fetcher content.Fetcher, desc ocispec.Descriptor) ([]ocispec.Descriptor, error) {
			nodes, err := content.Successors(ctx, fetcher, desc)
			if err != nil {
				return nil, err
			}
			var ret []ocispec.Descriptor
			for _, node := range nodes {
				if utils.SliceContains(toPull, node.Annotations[ocispec.AnnotationTitle]) {
					ret = append(ret, node)
				} else {
					estimatedBytes -= node.Size
				}
			}
			return ret, nil
		}

estimatedBytes is used in our byte by byte progress bar for download / upload progress

@Noxsios Noxsios closed this May 9, 2023
@Noxsios Noxsios deleted the make-err-skip-desc-public branch May 9, 2023 05:24
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants