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

match.Platforms and match.Digests #841

Merged
merged 1 commit into from Dec 8, 2020

Conversation

deitch
Copy link
Collaborator

@deitch deitch commented Nov 24, 2020

Option to:

  • match.Platforms([]v1.Platform) for any one of them.
  • match.Digest(v1.Hash)
  • match.Digests([]v1.Hash) for any one of them.

Also tests, of course.

@deitch deitch changed the title match.Platforms match.Platforms and match.Digests Nov 25, 2020
@deitch
Copy link
Collaborator Author

deitch commented Nov 25, 2020

Updated to include match.Digest and match.Digests

@deitch deitch mentioned this pull request Nov 25, 2020
@codecov-io
Copy link

codecov-io commented Nov 25, 2020

Codecov Report

Merging #841 (3ff3c6f) into master (cf9d7b3) will decrease coverage by 0.38%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #841      +/-   ##
==========================================
- Coverage   74.72%   74.33%   -0.39%     
==========================================
  Files         106      106              
  Lines        4427     4458      +31     
==========================================
+ Hits         3308     3314       +6     
- Misses        629      651      +22     
- Partials      490      493       +3     
Impacted Files Coverage Δ
pkg/v1/match/match.go 100.00% <100.00%> (ø)
pkg/v1/remote/transport/useragent.go 47.61% <0.00%> (-52.39%) ⬇️
pkg/crane/options.go 47.36% <0.00%> (-21.87%) ⬇️
pkg/v1/remote/transport/ping.go 86.95% <0.00%> (-8.70%) ⬇️
pkg/v1/remote/options.go 54.54% <0.00%> (-7.00%) ⬇️
pkg/gcrane/copy.go 84.37% <0.00%> (-1.14%) ⬇️
pkg/v1/remote/descriptor.go 73.78% <0.00%> (-0.61%) ⬇️
pkg/v1/remote/transport/basic.go 100.00% <0.00%> (ø)
pkg/v1/remote/transport/transport.go 100.00% <0.00%> (ø)
pkg/v1/remote/write.go 65.59% <0.00%> (+0.34%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf9d7b3...3ff3c6f. Read the comment docs.

@deitch
Copy link
Collaborator Author

deitch commented Dec 2, 2020

Review, please @jonjohnsonjr ?

@jonjohnsonjr
Copy link
Collaborator

I'm kinda -1 on having both match.Foo and match.Foos for everything...

Option 1:

func Foo(foo Foo) Matcher
func Foos(foos []Foo) Matcher

Option 2:

// Clients have to use match.Foos([]Foo{foo})
func Foos(foos []Foo) Matcher

Option 3:

// Works with match.Foos(foo) and match.Foos(foo, bar)
func Foos(foos ...Foo) Matcher

Option 4:

// Works with match.Foo(foo) and match.Foo(foo, bar)
func Foo(foos ...Foo) Matcher

WDYT? Order of preference for me is probably 4, 3, 1, 2.

We already have match.MediaTypes for 2, but this has been merged for such a short window that I'd be fine making a breaking change for that to make it consistent.

@deitch
Copy link
Collaborator Author

deitch commented Dec 8, 2020

Oh, yeah, I like 4. Variadic is so nice and easy. The question is, should it be named:

func Foo(foo Foo...) Matcher

or

func Foos(foo Foo...) Matcher

I think the second, since it can match multiple?

@jonjohnsonjr
Copy link
Collaborator

They're about equivalent to me, so whichever you prefer.

Signed-off-by: Avi Deitcher <avi@deitcher.net>
@deitch
Copy link
Collaborator Author

deitch commented Dec 8, 2020

OK, updated @jonjohnsonjr ; take a look. I did MediaTypes too, as you suggested.

@jonjohnsonjr jonjohnsonjr merged commit a33895c into google:master Dec 8, 2020
@deitch deitch deleted the with-multiple-platforms branch December 9, 2020 05:06
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.

None yet

3 participants