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

Add tests for tags list pagination #3422

Merged
merged 1 commit into from Jun 2, 2021

Conversation

joaodrp
Copy link
Collaborator

@joaodrp joaodrp commented May 22, 2021

Adds test coverage for #3143.

@joaodrp joaodrp self-assigned this May 22, 2021
Signed-off-by: João Pereira <484633+joaodrp@users.noreply.github.com>
@joaodrp
Copy link
Collaborator Author

joaodrp commented May 22, 2021

cc @eyJhb

@codecov-commenter
Copy link

Codecov Report

Merging #3422 (8ef268d) into main (d80a63f) will increase coverage by 0.19%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3422      +/-   ##
==========================================
+ Coverage   56.16%   56.36%   +0.19%     
==========================================
  Files         102      102              
  Lines        7303     7303              
==========================================
+ Hits         4102     4116      +14     
+ Misses       2553     2538      -15     
- Partials      648      649       +1     
Impacted Files Coverage Δ
...istribution/distribution/registry/handlers/tags.go 71.11% <0.00%> (+31.11%) ⬆️

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 d80a63f...8ef268d. Read the comment docs.

Copy link
Member

@milosgajdos milosgajdos left a comment

Choose a reason for hiding this comment

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

LGTM.

This kinda made me wonder if all those URLBuilder methods should provide variadic url.Value params in v3.

Yes I understand that would mean breaking notifications.URLBuilder, but since we've decided to break some interface in v3 we might as well do this.

@joaodrp
Copy link
Collaborator Author

joaodrp commented May 22, 2021

LGTM.

This kinda made me wonder if all those URLBuilder methods should provide variadic url.Value params in v3.

Yes I understand that would mean breaking notifications.URLBuilder, but since we've decided to break some interface in v3 we might as well do this.

That's a good point. However, do we have a use case (now or planned for the near future) for URL values in other methods that don't have them already? Exposing these before they're needed is counter-intuitive IMO.

@milosgajdos
Copy link
Member

We don't actually, no. I was merely thinking out loud just in case the topic comes up.

@joaodrp joaodrp requested a review from thaJeztah May 27, 2021 23:14
@milosgajdos milosgajdos added the conformance Related to conformance to distribution specification label May 28, 2021
@milosgajdos milosgajdos added this to the Registry/3.0.0 milestone May 28, 2021
@milosgajdos milosgajdos requested a review from waynr May 28, 2021 15:53
Copy link
Collaborator

@wy65701436 wy65701436 left a comment

Choose a reason for hiding this comment

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

lgtm

@milosgajdos milosgajdos merged commit 4f27e19 into distribution:main Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conformance Related to conformance to distribution specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants