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

adds support for the /v2/_catalog API #548

Merged
merged 10 commits into from
Oct 25, 2019
70 changes: 70 additions & 0 deletions pkg/v1/remote/catalog.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Copyright 2018 Google LLC All Rights Reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: 2019

//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package remote

import (
"encoding/json"
"fmt"
"net/http"
"net/url"

"github.com/google/go-containerregistry/pkg/name"
"github.com/google/go-containerregistry/pkg/v1/remote/transport"
)

type catalog struct {
Repos []string `json:"repositories"`
}

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

Choose a reason for hiding this comment

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

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?

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 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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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.

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 a synchronous callback - not a async one. The next page wouldn't be fetched until the callback returns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

o, err := makeOptions(target, options...)
if err != nil {
return nil, err
}

scopes := []string{target.Scope(transport.PullScope)}
tr, err := transport.New(target, o.auth, o.transport, scopes)
if err != nil {
return nil, err
}

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

uri := url.URL{
Scheme: target.Scheme(),
Host: target.RegistryStr(),
Path: "/v2/_catalog",
RawQuery: query,
}

client := http.Client{Transport: tr}
resp, err := client.Get(uri.String())
if err != nil {
return nil, err
}
defer resp.Body.Close()

if err := transport.CheckError(resp, http.StatusOK); err != nil {
return nil, err
}

var parsed catalog
if err := json.NewDecoder(resp.Body).Decode(&parsed); err != nil {
return nil, err
}

return parsed.Repos, nil
}
84 changes: 84 additions & 0 deletions pkg/v1/remote/catalog_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
// Copyright 2018 Google LLC All Rights Reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: 2019

//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package remote

import (
"net/http"
"net/http/httptest"
"net/url"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/google/go-containerregistry/pkg/authn"
"github.com/google/go-containerregistry/pkg/name"
)

func TestGetCatalog(t *testing.T) {
cases := []struct {
name string
responseBody []byte
wantErr bool
wantRepos []string
}{{
name: "success",
responseBody: []byte(`{"repositories":["test/test","foo/bar"]}`),
wantErr: false,
wantRepos: []string{"test/test", "foo/bar"},
}, {
name: "not json",
responseBody: []byte("notjson"),
wantErr: true,
}}
//TODO: add test cases for pagination
Copy link
Collaborator

Choose a reason for hiding this comment

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

Planning to do this later? 😄

supernit: add a space between // and TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?


for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
catalogPath := "/v2/_catalog"
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.URL.Path {
case "/v2/":
w.WriteHeader(http.StatusOK)
case catalogPath:
if r.Method != http.MethodGet {
t.Errorf("Method; got %v, want %v", r.Method, http.MethodGet)
}

w.Write(tc.responseBody)
default:
t.Fatalf("Unexpected path: %v", r.URL.Path)
}
}))
defer server.Close()
u, err := url.Parse(server.URL)
if err != nil {
t.Fatalf("url.Parse(%v) = %v", server.URL, err)
}

reg, err := name.NewRegistry(u.Host)
if err != nil {
t.Fatalf("name.NewRegistry(%v) = %v", u.Host, err)
}

repos, err := GetCatalog(reg, "", 100, WithAuthFromKeychain(authn.DefaultKeychain))
if (err != nil) != tc.wantErr {
t.Errorf("GetCatalog() wrong error: %v, want %v: %v\n", (err != nil), tc.wantErr, err)
}

if diff := cmp.Diff(tc.wantRepos, repos); diff != "" {
t.Errorf("GetCatalog() wrong repos (-want +got) = %s", diff)
}
})
}
}