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

implement listTags tests #918

Merged
merged 1 commit into from
Feb 11, 2021
Merged

Conversation

cpanato
Copy link
Contributor

@cpanato cpanato commented Jan 19, 2021

as a continuation of in intention of the work here: #915

This PR implements the ListTags tests

All feedback are welcome

/assign @jonjohnsonjr

countTags := 0
for tag := range c {
if countTags >= n {
break
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd be satisfied with just a TODO here that we need to include the Link header for the next page, but you could also just implement it, if you'd like!

https://github.com/opencontainers/distribution-spec/blob/b505e9cc53ec499edbd9c1be32298388921bb705/detail.md#tags-paginated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonjohnsonjr thanks for the feedback, i will implement that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to run the list tags against docker API and I think I'm doing something wrong because when I set the n to be something like 5 or 100 it returns the same as not setting the n

TOKEN=$(curl -s "https://auth.docker.io/token?scope=repository%3Alibrary%2Fgolang%3Apull&service=registry.docker.io" | jq -r .token)
curl -v -s -H "Authorization: Bearer ${TOKEN}" "https://index.docker.io/v2/library/golang/tags/list?n=5"

am I doing something very wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried in gcr as well same results :/

Copy link
Collaborator

Choose a reason for hiding this comment

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

am I doing something very wrong?

Nope! It's just that neither docker hub nor GCR implement this (for different reasons).

Looking for a random public image on the internet, it seems that quay.io does implement this:

$ curl -v https://quay.io/v2/kubernetes_incubator/nfs-provisioner/tags/list?n=5

Copy link
Contributor Author

@cpanato cpanato Feb 11, 2021

Choose a reason for hiding this comment

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

@jonjohnsonjr I added a TODO, because I'm afraid I cannot implement this due to lack of time and would not have this PR hanging around for a long time.

When I have more time I will come back if this was not implemented by another person.

PTAL

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good, thanks!

@codecov-io
Copy link

codecov-io commented Jan 19, 2021

Codecov Report

Merging #918 (69dcf19) into master (213dd48) will increase coverage by 0.12%.
The diff coverage is 86.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #918      +/-   ##
==========================================
+ Coverage   72.77%   72.90%   +0.12%     
==========================================
  Files         108      108              
  Lines        4713     4757      +44     
==========================================
+ Hits         3430     3468      +38     
- Misses        773      778       +5     
- Partials      510      511       +1     
Impacted Files Coverage Δ
pkg/registry/manifest.go 87.67% <85.71%> (-0.80%) ⬇️
pkg/registry/registry.go 100.00% <100.00%> (ø)

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 213dd48...69dcf19. Read the comment docs.

Base automatically changed from master to main January 27, 2021 18:57
Signed-off-by: Carlos Panato <ctadeu@gmail.com>
countTags := 0
for tag := range c {
if countTags >= n {
break
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good, thanks!

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