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.

Sign up for GitHub

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: Allow configuring the logger #521

Closed
wants to merge 1 commit into from
Closed

registry: Allow configuring the logger #521

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 37 additions & 10 deletions pkg/registry/registry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,19 +28,20 @@ import (
"net/http"
)

type v struct {
type registry struct {
log *log.Logger
blobs blobs
manifests manifests
}

// https://docs.docker.com/registry/spec/api/#api-version-check
// https://github.com/opencontainers/distribution-spec/blob/master/spec.md#api-version-check
func (v *v) v2(resp http.ResponseWriter, req *http.Request) *regError {
func (r *registry) v2(resp http.ResponseWriter, req *http.Request) *regError {
if isBlob(req) {
return v.blobs.handle(resp, req)
return r.blobs.handle(resp, req)
}
if isManifest(req) {
return v.manifests.handle(resp, req)
return r.manifests.handle(resp, req)
}
resp.Header().Set("Docker-Distribution-API-Version", "registry/2.0")
if req.URL.Path != "/v2/" && req.URL.Path != "/v2" {
Expand All @@ -54,18 +55,44 @@ func (v *v) v2(resp http.ResponseWriter, req *http.Request) *regError {
return nil
}

func (v *v) root(resp http.ResponseWriter, req *http.Request) {
if rerr := v.v2(resp, req); rerr != nil {
log.Printf("%s %s %d %s %s", req.Method, req.URL, rerr.Status, rerr.Code, rerr.Message)
func (r *registry) root(resp http.ResponseWriter, req *http.Request) {
if rerr := r.v2(resp, req); rerr != nil {
r.logf("%s %s %d %s %s", req.Method, req.URL, rerr.Status, rerr.Code, rerr.Message)
rerr.Write(resp)
return
}
log.Printf("%s %s", req.Method, req.URL)
r.logf("%s %s", req.Method, req.URL)
}

// New returns a handler which implements the docker registry protocol. It should be registered at the site root.
func (r *registry) logf(f string, v ...interface{}) {
if r.log == nil {
log.Printf(f, v...)
} else {
r.log.Printf(f, v...)
}
}

// New returns a handler which implements the docker registry protocol.
// It should be registered at the site root.
func New() http.Handler {
v := v{
return NewWithOptions(nil)
}

// Options describes the available options
// for creating the registry.
type Options struct {
// Log is used to log requests.
// If nil, the global logger is used.
Log *log.Logger
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks awesome - I think this is definitely a pretty reasonable thing to change and thank you for sending in the change.

They style in go is generally to define a option type - i.e.

type Option func(o *registry)

And then have New take variadic arguments - i.e.
New(o Option...)

and in the body do something like

opts := &registry{log:defaultLog}
for _, o := range options {
o(opts)
}

And define the logger option like
func WithLogger(l log.Type) Option

so you can call New() to get default behaviour, New(WithLogger(log.SomethingComplicated)) to get more interesting behaviour.

Let me know if that makes sense - I'm happy to have the log be configurable, just want to make sure the option pattern is nice for the user. I'm also happy to make these changes if I'm asking for too much work from you.

Copy link
Author

Choose a reason for hiding this comment

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

They style in go is generally to define a option type - i.e.

Its another pattern for sure but I wouldn't say its generally better. The stdlib for example doesn't use it at all and even though later packages did, the new go-cloud package made by the Go team explicitly decided against using them in favour of optional structs.

See google/go-cloud#908 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

I'm also happy to make these changes if I'm asking for too much work from you.

Feel free to take over.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think both approaches are reasonable. There are definitely some hard to solve problems with the functional options approach, but it looks so much nicer in some cases 😄

We should keep in mind any other extension points e.g. #488

Copy link
Contributor

Choose a reason for hiding this comment

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

I pushed a version of this in #537 - let me know what you think :D


// NewWithOptions is the same as New but takes options.
func NewWithOptions(opts *Options) http.Handler {
if opts == nil {
opts = &Options{}
}
v := registry{
log: opts.Log,
blobs: blobs{
contents: map[string][]byte{},
uploads: map[string][]byte{},
Expand Down