adds support for the /v2/_catalog API #548

jmakinen-ncc · 2019-09-20T22:16:12Z

No description provided.

googlebot · 2019-09-20T22:16:15Z

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

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

jmakinen-ncc · 2019-09-20T22:21:06Z

CLA-wise I just use a different email for github than what I used to commit that

jonjohnsonjr · 2019-09-20T22:22:15Z

pkg/v1/remote/catalog.go

+}
+
+// GetCatalog calls /_catalog, returning the list of repositories on the registry
+func GetCatalog(target name.Registry, options ...Option) ([]string, error) {


Given that the results of catalog can be paginated, there might be a better return type than []string -- I'm not sure what the most go idiomatic thing to do here is...

We could return a channel? Or do callbacks for each "page"?

Or maybe this is just fine...

what about now? (sorry to double comment)

I think it'd make sense for GetCatalog to do pagination itself, and return all the results it could find from however many requests were necessary to find them. That seems simpler than making callers deal with pagination, or pass a channel, or whatever.

codecov-io · 2019-09-20T22:24:20Z

Codecov Report

Merging #548 into master will increase coverage by 0.52%.
The diff coverage is 41.46%.

@@            Coverage Diff             @@
##           master     #548      +/-   ##
==========================================
+ Coverage   72.34%   72.87%   +0.52%     
==========================================
  Files          95      102       +7     
  Lines        4245     4486     +241     
==========================================
+ Hits         3071     3269     +198     
- Misses        777      806      +29     
- Partials      397      411      +14

Impacted Files	Coverage Δ
pkg/crane/catalog.go	`0% <0%> (ø)`
pkg/v1/remote/catalog.go	`68% <68%> (ø)`
pkg/v1/google/auth.go	`71.42% <0%> (-15.42%)`	⬇️
pkg/authn/auth.go	`100% <0%> (ø)`	⬆️
pkg/v1/tarball/layer.go	`77.77% <0%> (ø)`	⬆️
pkg/authn/basic.go	`100% <0%> (ø)`	⬆️
pkg/authn/bearer.go	`100% <0%> (ø)`	⬆️
pkg/authn/helper.go
pkg/v1/remote/transport/logger.go	`100% <0%> (ø)`
... and 24 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 53e1ac5...7f18e32. Read the comment docs.

googlebot · 2019-09-20T22:39:54Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

jmakinen-ncc · 2019-09-20T22:40:53Z

How about now?

imjasonh · 2019-09-20T23:38:59Z

pkg/v1/remote/catalog.go

+
+	var query string
+
+	query = fmt.Sprintf("last=%s&n=%d", url.QueryEscape(last), n)


query := fmt.Sprintf(...)

imjasonh · 2019-09-20T23:39:07Z

pkg/v1/remote/catalog.go

+
+// GetCatalog calls /_catalog, returning the list of repositories on the registry
+func GetCatalog(target name.Registry, last string, n int, options ...Option) ([]string, error) {
+


nit: unnecessary blank line

imjasonh · 2019-09-20T23:39:25Z

pkg/v1/remote/catalog.go

+		return nil, err
+	}
+
+	//TKTK:JM iterate through results with "last"


Is this meant to be a TODO?

imjasonh · 2019-09-20T23:39:39Z

pkg/v1/remote/catalog.go

+	return parsed.Repos, nil
+}
+
+//TKTK:JM write tests


Yes, please do. 😄

imjasonh · 2019-09-20T23:40:19Z

pkg/v1/remote/catalog.go

+		return nil, err
+	}
+
+	parsed := catalog{}


supernit: I typically see this as var parsed catalog which has the same effect but is infinitesimally more idiomatic.

jmakinen-ncc · 2019-09-21T00:24:44Z

Thanks for the comments, back atcha

imjasonh · 2019-09-21T01:31:56Z

pkg/v1/remote/catalog.go

+}
+
+// GetCatalog calls /_catalog, returning the list of repositories on the registry
+func GetCatalog(target name.Registry, last string, n int, options ...Option) ([]string, error) {


How do you feel about GetCatalog just doing pagination itself and returning all the values it can find? How many pages of results do we expect to get from this? Tens of thousands?

That's why I was hesitant to implement it because I'm not sure of the scale.
I was planning on writing a helper for my own purposes that I can add later, but I just needed this at a minimum.

The scale might be a concern, but for (most?) users who should expect only a couple pages, it sucks that this makes them handle pagination themselves.

I'd prefer to expose the pagination-handling version for the users that don't want to deal with it, and maybe if we hear that this method is a pain for whatever scaling reason, we can export the DIY method later. If we never hear that we need to expose the guts, that's great.

Does that make any sense?

If we're going to handle the pagination internally, we should probably make GetCatalog take a context.Context so that you can cancel it.

it sucks that this makes them handle pagination themselves

This is basically why the crane package exists. IMO we should (within reason) expose all the complexity that the API itself has, then we can expose the happy path in crane.

Another pattern I've seen that is nice, is to have the GetCatalog take a callback function that gets called per page. It allows users to abort early (via returning an error / cancelling the context), but it hides the pagination detail from the user.

@imjasonh thinks they're not idiomatic go. There's this presentation for inspiration.

I'm not sure if we could borrow something from that, but it also doesn't like callbacks (from here):

Bryan briefly mentions asynchronous callbacks as something programmers from other certain languages sometimes try to use. But he notes that most Go programmers already know not to use them in Go, and moves on to talking about Futures and Producer–Consumer Queues.

This is a synchronous callback - not a async one. The next page wouldn't be fetched until the callback returns.

Ok, how do we feel about my new crane.GetCatalog?

jonjohnsonjr · 2019-09-23T18:33:37Z

@jmakinen-ncc so, let's just punt on figuring out a reasonable API for paginated requests. I'd say just add your original, easy-to-use implementation under pkg/crane for now. We can come up with a better way to expose pagination later, and we'll actually want to do that for listing tags as well. Filed #549 to track that.

Sorry for the back and forth!

jmakinen-ncc · 2019-09-23T19:16:31Z

Sorry, so does that mean that this can be merged?

… registry

clrprod · 2019-10-02T18:01:22Z

pkg/v1/remote/catalog.go

+}
+
+// GetCatalogPage calls /_catalog, returning the list of repositories on the registry
+func GetCatalogPage(target name.Registry, last string, n int, options ...Option) ([]string, error) {


I think the idea is to just make this a private method in the crane/catalog.go file. At some point in the future it can get moved to the remote library when we have interface concensus.

I think I'm actually okay with leaving this in. If we ever come up with a good API for this, we can name it GetCatalog, and GetCatalogPage would still be useful.

I am a little bit unhappy with this because it doesn't allow clients to adhere to the spec:

Compliant client implementations should always use the Link header value when proceeding through results linearly.

However, this does enable you to skip ahead, which is immediately after:

The client may construct URLs to skip forward in the catalog.

I think we're basically making it impossible (sometimes, depends on implementation) for a registry to implement this efficiently by handing us a cursor in the Link header, but maybe we don't care.

jmakinen-ncc · 2019-10-03T00:10:53Z

Would anyone be able to help me figure out why the build is failing here?
do I need to write docs for the added command or something?
Also, I feel like keeping the most generic interface in the remote package and then having the pretty one in crane makes more sense right?
So that if anyone wants to do the dirty work of working with pages directly but still having the benefits of having auth and everything handled then they can. I would think, we'd want remote to be the most 1:1 representation of the API possible
but if someone just wants a full list without having to think, they can call the user-friendly crane

jonjohnsonjr · 2019-10-03T17:41:35Z

Would anyone be able to help me figure out why the build is failing here?
do I need to write docs for the added command or something?

It's complaining that you haven't run ./hack/update-code-gen.sh to generate the docs. We could probably make that more evident.

Also, I feel like keeping the most generic interface in the remote package and then having the pretty one in crane makes more sense right?
So that if anyone wants to do the dirty work of working with pages directly but still having the benefits of having auth and everything handled then they can. I would think, we'd want remote to be the most 1:1 representation of the API possible

Yeah, definitely. I'm just not sure if this is the API we want to support forever. It has the same drawback as this, where we can't reuse the auth handshake results across multiple pages. I'm also not sure if we want to return just the []string or the whole struct. There's some other stuff we're not really exposing here (e.g. the Link header, which seems like we should respect it as a client, and there's a next field in the response there which seems like a bug in the spec actually).

On the other hand, the crane API is pretty straightforward, so I think it's reasonable to merge that now (so you can use it) until we can figure out the best way to represent this. Do you need the ability to list individual pages? Or is the crane API sufficient for now?

jonjohnsonjr · 2019-10-03T17:41:55Z

cmd/crane/cmd/catalog.go

+// NewCmdGetCatalog creates a new cobra.Command for the repos subcommand.
+func NewCmdGetCatalog() *cobra.Command {
+	return &cobra.Command{
+		Use:   "repos",


I think I'd rather this just be catalog than repos

jonjohnsonjr · 2019-10-18T21:00:43Z

@jmakinen-ncc any interest in pushing this over the finish line? I feel like it's pretty close to merge-able.

jmakinen-ncc · 2019-10-18T22:23:08Z

Yeah, sorry been looking for the free time in my personal life to close up some loose threads. Will do asap. On Oct 18, 2019 2:00 PM, jonjohnsonjr <[email protected]> wrote: @jmakinen-ncc<https://github.com/jmakinen-ncc> any interest in pushing this over the finish line? I feel like it's pretty close to merge-able. - You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#548?email_source=notifications&email_token=AGKDKP3JOEHHVXTGNYM3A5TQPIPXZA5CNFSM4IY3XYZ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBV6Z7Y#issuecomment-543943935>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGKDKP2V4D6YZAX7U37KODTQPIPXZANCNFSM4IY3XYZQ>.

jonjohnsonjr · 2019-10-21T17:08:49Z

sorry

No worries! I was just running through old issues and PRs trying to clean things up, there's no rush 😄

…repos

jmakinen-ncc · 2019-10-21T22:00:14Z

Do we have a winner?

jonjohnsonjr · 2019-10-21T22:31:44Z

cmd/crane/doc/crane_repos.md

@@ -0,0 +1,22 @@
+## crane repos


nit: drop this file

jonjohnsonjr · 2019-10-21T23:03:17Z

cmd/crane/cmd/catalog.go

+func init() { Root.AddCommand(NewCmdGetCatalog()) }
+
+// NewCmdGetCatalog creates a new cobra.Command for the repos subcommand.
+func NewCmdGetCatalog() *cobra.Command {


Nit: I'd name this "NewCmdCatalog" to be consistent with the rest of the commands.

jonjohnsonjr · 2019-10-21T23:05:19Z

pkg/crane/catalog.go

+	"github.com/google/go-containerregistry/pkg/v1/remote"
+)
+
+// GetCatalog returns the repositories in a registry's catalog


supernit: add a period at the end of this sentence

jonjohnsonjr · 2019-10-21T23:05:25Z

pkg/crane/catalog.go

+	n := 100
+	last := ""
+	for {
+


supernit: drop this line

jonjohnsonjr · 2019-10-21T23:05:33Z

pkg/crane/catalog.go

+		if len(page) < n {
+			break
+		}
+


supernit: drop this line

jonjohnsonjr · 2019-10-21T23:06:13Z

pkg/v1/remote/catalog.go

+	Repos []string `json:"repositories"`
+}
+
+// GetCatalogPage calls /_catalog, returning the list of repositories on the registry


supernit: add a period at the end of this sentence

jonjohnsonjr · 2019-10-21T23:07:09Z

pkg/v1/remote/catalog.go

+}
+
+// GetCatalogPage calls /_catalog, returning the list of repositories on the registry
+func GetCatalogPage(target name.Registry, last string, n int, options ...Option) ([]string, error) {


I think I'm actually okay with leaving this in. If we ever come up with a good API for this, we can name it GetCatalog, and GetCatalogPage would still be useful.

I am a little bit unhappy with this because it doesn't allow clients to adhere to the spec:

Compliant client implementations should always use the Link header value when proceeding through results linearly.

However, this does enable you to skip ahead, which is immediately after:

The client may construct URLs to skip forward in the catalog.

I think we're basically making it impossible (sometimes, depends on implementation) for a registry to implement this efficiently by handing us a cursor in the Link header, but maybe we don't care.

jonjohnsonjr · 2019-10-21T23:13:18Z

pkg/v1/remote/catalog_test.go

+		responseBody: []byte("notjson"),
+		wantErr:      true,
+	}}
+	//TODO: add test cases for pagination


Planning to do this later? 😄

supernit: add a space between // and TODO

Potentially, but I just wanted to note that they are missing if anyone runs into issues there later and wants to write some.
Do you need this?

jonjohnsonjr

LGTM, thanks!

Resolved

jmakinen-ncc · 2019-10-22T17:52:16Z

Thanks everyone!

jonjohnsonjr

@jmakinen-ncc sorry I don't mean to renege, but I just saw these final nits right before I hit merge 😅

The nits mean we care ❤️

jonjohnsonjr · 2019-10-22T17:58:57Z

cmd/crane/cmd/catalog.go

+
+func init() { Root.AddCommand(NewCmdCatalog()) }
+
+// NewCmdGetCatalog creates a new cobra.Command for the repos subcommand.


Nit: NewCmdCatalog

jonjohnsonjr · 2019-10-22T17:59:16Z

pkg/crane/catalog.go

+	"github.com/google/go-containerregistry/pkg/v1/remote"
+)
+
+// GetCatalog returns the repositories in a registry's catalog.


Can You change these to just Catalog?

jonjohnsonjr · 2019-10-22T17:59:26Z

pkg/v1/remote/catalog.go

+	Repos []string `json:"repositories"`
+}
+
+// GetCatalogPage calls /_catalog, returning the list of repositories on the registry.


Can You change these to just CatalogPage?

jonjohnsonjr · 2019-10-22T17:59:39Z

pkg/v1/remote/catalog_test.go

+	"github.com/google/go-containerregistry/pkg/name"
+)
+
+func TestGetCatalogPage(t *testing.T) {


TestCatalogPage

jonjohnsonjr · 2019-10-22T18:00:20Z

cmd/crane/cmd/catalog.go

@@ -0,0 +1,45 @@
+// Copyright 2018 Google LLC All Rights Reserved.


jonjohnsonjr · 2019-10-22T18:00:27Z

pkg/crane/catalog.go

@@ -0,0 +1,49 @@
+// Copyright 2018 Google LLC All Rights Reserved.


jonjohnsonjr · 2019-10-22T18:00:33Z

pkg/v1/remote/catalog.go

@@ -0,0 +1,70 @@
+// Copyright 2018 Google LLC All Rights Reserved.


jonjohnsonjr · 2019-10-22T18:00:39Z

pkg/v1/remote/catalog_test.go

@@ -0,0 +1,83 @@
+// Copyright 2018 Google LLC All Rights Reserved.


…ity in!(no worries if I'm still not there tho)

jonjohnsonjr

(no worries if I'm still not there tho)

😆

git is my favorite chat protocol

Thanks! Sorry again for all the back and forth :)

jmakinen-ncc · 2019-10-25T19:31:38Z

git is my favorite chat protocol

Ask @clrprod about "Big changes, no promises"

This reverts commit f9947dc.

* Revert "adds support for the /v2/_catalog API (#548)" This reverts commit f9947dc. * Re-apply catalog changes

adds support for the /v2/_catalog API

2c7a9fe

jonjohnsonjr reviewed Sep 20, 2019

View reviewed changes

adds pagination to registry catalog function

c765a42

imjasonh previously requested changes Sep 20, 2019

View reviewed changes

Addresses review comments and adds tests for catalog request feature

c4425a3

imjasonh reviewed Sep 21, 2019

View reviewed changes

jmakinen-ncc requested a review from imjasonh September 25, 2019 17:44

adds a helper function in crane to retrieve all repos from the remote…

11dd278

… registry

clrprod reviewed Oct 2, 2019

View reviewed changes

jonjohnsonjr mentioned this pull request Oct 2, 2019

google.WalkFunc should be able to be parallelized #556

Closed

jmakinen-ncc added 2 commits October 2, 2019 23:29

fixes the testcase name

023e4fd

fixes test cases for GetCatalogPage

5c0ed6d

jonjohnsonjr reviewed Oct 3, 2019

View reviewed changes

runs the codegen script and changes the command name to catalog from …

ba3476b

…repos

jonjohnsonjr reviewed Oct 21, 2019

View reviewed changes

addresses review comments

4edb362

addresses another supernit

7f18e32

jonjohnsonjr approved these changes Oct 22, 2019

View reviewed changes

jmakinen-ncc closed this Oct 22, 2019

jmakinen-ncc reopened this Oct 22, 2019

jonjohnsonjr requested changes Oct 22, 2019

View reviewed changes

fixes the hopefully last round of nits to get this catalog functional…

9eeee72

…ity in!(no worries if I'm still not there tho)

jonjohnsonjr approved these changes Oct 25, 2019

View reviewed changes

jonjohnsonjr merged commit f9947dc into google:master Oct 25, 2019

jonjohnsonjr mentioned this pull request Nov 11, 2019

Use Link header for pagination #607

Merged

jonjohnsonjr added a commit to jonjohnsonjr/go-containerregistry that referenced this pull request Nov 15, 2019

Revert "adds support for the /v2/_catalog API (google#548)"

ba08d9a

This reverts commit f9947dc.

jonjohnsonjr mentioned this pull request Nov 15, 2019

Drop the weird git submodule for deepcopy-gen #613

Merged

jonjohnsonjr added a commit that referenced this pull request Nov 15, 2019

Drop the weird git submodule for deepcopy-gen (#613)

c0886fb

* Revert "adds support for the /v2/_catalog API (#548)" This reverts commit f9947dc. * Re-apply catalog changes

jonjohnsonjr mentioned this pull request Feb 1, 2022

Feature Request: Consider add concurrency and verbosity for remote.Catalog #1278

Closed


		var query string

		query = fmt.Sprintf("last=%s&n=%d", url.QueryEscape(last), n)


		// GetCatalog calls /_catalog, returning the list of repositories on the registry
		func GetCatalog(target name.Registry, last string, n int, options ...Option) ([]string, error) {


		func init() { Root.AddCommand(NewCmdCatalog()) }

		// NewCmdGetCatalog creates a new cobra.Command for the repos subcommand.

		@@ -0,0 +1,45 @@
		// Copyright 2018 Google LLC All Rights Reserved.

		@@ -0,0 +1,49 @@
		// Copyright 2018 Google LLC All Rights Reserved.

		@@ -0,0 +1,70 @@
		// Copyright 2018 Google LLC All Rights Reserved.

		@@ -0,0 +1,83 @@
		// Copyright 2018 Google LLC All Rights Reserved.

adds support for the /v2/_catalog API #548

adds support for the /v2/_catalog API #548

Conversation

jmakinen-ncc commented Sep 20, 2019

googlebot commented Sep 20, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

jmakinen-ncc commented Sep 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 20, 2019 • edited Loading

Codecov Report

googlebot commented Sep 20, 2019

jmakinen-ncc commented Sep 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmakinen-ncc commented Sep 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonjohnsonjr commented Sep 23, 2019

jmakinen-ncc commented Sep 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmakinen-ncc commented Oct 3, 2019

jonjohnsonjr commented Oct 3, 2019

Choose a reason for hiding this comment

jonjohnsonjr commented Oct 18, 2019

jmakinen-ncc commented Oct 18, 2019 via email

jonjohnsonjr commented Oct 21, 2019

jmakinen-ncc commented Oct 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonjohnsonjr left a comment

Choose a reason for hiding this comment

jmakinen-ncc commented Oct 22, 2019

jonjohnsonjr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonjohnsonjr left a comment

Choose a reason for hiding this comment

jmakinen-ncc commented Oct 25, 2019

codecov-io commented Sep 20, 2019 •

edited

Loading