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

registry: Add ListManfiests and ListRepositoriesV2 api endpoint support + Token pagination #501

Merged
merged 8 commits into from
Dec 3, 2021
Merged

registry: Add ListManfiests and ListRepositoriesV2 api endpoint support + Token pagination #501

merged 8 commits into from
Dec 3, 2021

Conversation

CollinShoop
Copy link
Contributor

@CollinShoop CollinShoop commented Nov 24, 2021

Changes

I did go ahead and create a throwaway project to test these new methods and they work as expected. The token pagination also works well 👍🏻

Follow up

  • Changelog / version bump
  • Add support to doctl

CON-3907

@CollinShoop CollinShoop marked this pull request as ready for review November 30, 2021 15:21
waynr
waynr previously approved these changes Nov 30, 2021
Copy link
Contributor

@waynr waynr left a comment

Choose a reason for hiding this comment

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

This generally looks good to me, but I was thinking it should probably include some additional tests in links_test.go and maybe godo_test.go.

It would also be good to validate these new endpoints in our private e2e repo where we have at least one test for listing repositories and probably another for listing images by tag. To test the changes in this repo you would use go mod edit -replace in your local copy of the e2e repo to point it at your local copy of this repo.

PerPage int `url:"per_page,omitempty"`

// For paginated result sets which support tokens, the token provided by the last set
// of results in order to retrieve the next set of results. This is expected to be faster
Copy link
Contributor

Choose a reason for hiding this comment

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

This is expected to be faster than incrementing or decrementing the page number.

Might be helpful here to link to a blog post or some other kind of documentation about how token-based pagination leads to more efficient database queries since it's not obvious at first glance why it would be faster. Then again, maybe because this is a public repo we don't need to worry about that kind of detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an implementation detail that I suspect may be different across teams and functions, so I'd rather keep it vague for now.

links.go Outdated Show resolved Hide resolved
@CollinShoop
Copy link
Contributor Author

CollinShoop commented Nov 30, 2021

@waynr

This generally looks good to me, but I was thinking it should probably include some additional tests in links_test.go and maybe godo_test.go.

Agreed, not sure how I missed that. Added 👍🏻

It would also be good to validate these new endpoints in our private e2e repo where we have at least one test for listing repositories and probably another for listing images by tag. To test the changes in this repo you would use go mod edit -replace in your local copy of the e2e repo to point it at your local copy of this repo.

That's also a good point. I've created CON-5667 to track that work independently. https://github.com/digitalocean/e2e/pull/228

Copy link
Contributor

@scotchneat scotchneat left a comment

Choose a reason for hiding this comment

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

Looks good overall. Just had some comments

links.go Outdated Show resolved Hide resolved
registry.go Outdated Show resolved Hide resolved
registry.go Outdated Show resolved Hide resolved
Copy link
Contributor

@scotchneat scotchneat left a comment

Choose a reason for hiding this comment

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

👍🏽

@CollinShoop
Copy link
Contributor Author

@andrewsomething Do you mind taking a look and merging? I'll follow this up with a version / changelog pr.

Copy link
Member

@andrewsomething andrewsomething left a comment

Choose a reason for hiding this comment

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

👍

@andrewsomething andrewsomething merged commit 1f3cbb4 into digitalocean:main Dec 3, 2021
@CollinShoop CollinShoop deleted the add-list-manifests-and-list-repositories-v2-api-endpoint-support branch December 3, 2021 19:20
@CollinShoop CollinShoop mentioned this pull request Dec 3, 2021
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.

5 participants