heroku / docker-registry-client

A Go API client for the v2 Docker Registry API
BSD 3-Clause "New" or "Revised" License
297 stars 225 forks source link

registry.NewQuiet: non-logging variant #3

Closed glasser closed 8 years ago

glasser commented 8 years ago

This commit allows you to use the library without logging.

ojacobson commented 8 years ago

I like the idea. I might adjust this to use a callback instead of littering the code with ifs:

var Quiet := func(format string, args... interface{}) {}
var Log := func(format string, args... interface{}) {
    log.Printf(format, args...)
}
…
registry := {
    …
    log: Quiet
}
…
registry.log("message")

Do you have a strong affinity for either implementation?

glasser commented 8 years ago

No strong feeling. I can make that change or you can.

BTW I don't think the "NewQuiet" pattern is great (vs just setting an exported field on the object) but it seemed the easiest way to do this backwards compatibly since you need to make the change before New's implicit Ping.

ojacobson commented 8 years ago

Yeah, NewQuiet appears to mean that every permutation of options would need its own factory function, which isn't great. What about something where the caller passes a series of opaque option values to New to assemble behaviours like this?

a := registry.New(url, username, password)
b := registry.New(url, username, password, registry.QuietLogging)
c := registry.New(url, username, password, registry.InsecureTransport)
d := registry.New(url, username, password, registry.QuietLogging, registry.InsecureTransport)
glasser commented 8 years ago

I mean I'd be happy to just directly create a Registry struct and have an easy way to get the triple Transport wrapping. (I'm fine with skipping the initial Ping if I do this... Happy even, saves me a few round trips fetching tokens which aren't useful for anything but Ping!) New would then just be a helper for the simple cases (which also does the Ping?)

ojacobson commented 8 years ago

Heh, yeah, this lib is very careless about token use. Easy to call, but very chatty as a consequence.

I'm keeping the Ping in New, at least: the application I originally wrote this for relies on that to warn the user about misconfiguration, since it runs as a webhook endpoint and otherwise has very few ways to interact with the user. Constructing a Registry struct by hand should already work; I'll split out some (public) factory functions for the transport stack, since that's by far the hairiest part of New. Seems like a reasonable set of ideas to me.

ojacobson commented 8 years ago

Have a look at #5, which includes your changes plus the above discussion.