Skip to content
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

Make ErrSchema1 checkable via errors.Is() #1721

Merged
merged 1 commit into from
Jun 14, 2023

Conversation

Laitr0n
Copy link
Contributor

@Laitr0n Laitr0n commented Jun 5, 2023

The TestGetSchema1 said it should fail based on media type while gave the unsupported MediaType. But it not return the true.

This fixes that problem by implementing an Is() function on ErrSchema1
so that errors.Is() can properly identify the error as an ErrSchema1.
Usage can now be: errors.Is(err, &ErrSchema1{schema : "foo"})

@google-cla
Copy link

google-cla bot commented Jun 5, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@Laitr0n Laitr0n marked this pull request as ready for review June 5, 2023 15:31
@imjasonh
Copy link
Collaborator

imjasonh commented Jun 5, 2023

cc @jonjohnsonjr

@codecov-commenter
Copy link

codecov-commenter commented Jun 5, 2023

Codecov Report

Merging #1721 (b75f624) into main (037ab31) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head b75f624 differs from pull request most recent head e81effa. Consider uploading reports for the commit e81effa to get more accurate results

@@            Coverage Diff             @@
##             main    #1721      +/-   ##
==========================================
+ Coverage   71.99%   72.04%   +0.04%     
==========================================
  Files         121      121              
  Lines        9768     9764       -4     
==========================================
+ Hits         7032     7034       +2     
+ Misses       2060     2056       -4     
+ Partials      676      674       -2     
Impacted Files Coverage Δ
pkg/v1/remote/descriptor.go 80.89% <100.00%> (-0.83%) ⬇️

... and 1 file with indirect coverage changes

Copy link
Collaborator

@jonjohnsonjr jonjohnsonjr left a comment

Choose a reason for hiding this comment

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

I think embedding the mediaType that we saw is both unnecessary and makes this really complicated.

Can you update this to just be:

var ErrSchema1 = errors.New("unsupported schema1 mediaType, see https://github.com/google/go-containerregistry/issues/377")

And fix any tests that it breaks?

@Laitr0n
Copy link
Contributor Author

Laitr0n commented Jun 5, 2023

I think embedding the mediaType that we saw is both unnecessary and makes this really complicated.

Can you update this to just be:

var ErrSchema1 = errors.New("unsupported schema1 mediaType, see https://github.com/google/go-containerregistry/issues/377")

And fix any tests that it breaks?

OK. I will done.

Copy link
Collaborator

@jonjohnsonjr jonjohnsonjr left a comment

Choose a reason for hiding this comment

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

Oh that's clever. Added a couple suggestions so that we can keep the exact same error string but have the %w wrapping make errors.Is work.

I have a feeling someone, somewhere is checking for the string value of this error specifically because errors.Is doesn't work, so I want to avoid breaking that.

pkg/v1/remote/descriptor.go Outdated Show resolved Hide resolved
pkg/v1/remote/descriptor.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@jonjohnsonjr jonjohnsonjr left a comment

Choose a reason for hiding this comment

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

thanks!

@jonjohnsonjr jonjohnsonjr merged commit 6de8d1d into google:main Jun 14, 2023
@Laitr0n Laitr0n deleted the is_errSchema1_fix branch June 15, 2023 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants