WIP: Docker tests and clean #30
Conversation
runcom
force-pushed
the
docker-tests
branch
2 times, most recently
from
5a8e692 to
7399b12
Compare
| func newDockerClient(refHostname, certPath string, tlsVerify bool) (*dockerClient, error) { | ||
| // NewClient returns a new Client instance for refHostname (a host a specified in the Docker image reference, not canonicalized to dockerRegistry) | ||
| func NewClient(img, certPath string, tlsVerify bool) (Client, error) { | ||
| ref, _, err := parseDockerImageName(img) |
There was a problem hiding this comment.
This is not so clean here, I would move the parseDockerImageName call up in the call stack but that could affect NewImageSource and NewImageSource as well, not sure. I believe we can live with this duplication but since NewClient is exported I'd rather have a better API here.
The duplication I'm talking about can be clearly seen above in doc.go where we pass img to both the client and the image creations.
After all, I personally don't think this duplication is that bad because Client is now an interface and how we build our own is our implementation detail in this library no? That said, I think NewClient should receive an hostname instead but with this we get back to my original statement to move parseDockerImageName up in the call stack.
@mtrmac WDYT?
There was a problem hiding this comment.
I’m thinking the client should be hidden from users of the library, and then AFAICS this problem disappears because the parsing would still happen in the Image{,Source,Destination}.
Separately, for policy namespacing I will probably need a transport-independent “parsed reference” (types.ImageReference interface), with external users first converting a string into an implementation of that interface (or perhaps constructing an implementation directly), and then calling NewImage$something; so the parsing would happen separately and externally.
There was a problem hiding this comment.
I’m thinking the client should be hidden from users of the library
that's how it is now, but letting ppl specify a custom client if they wish is nice leveraging golang interfaces
runcom
force-pushed
the
docker-tests
branch
3 times, most recently
from
34c0b81 to
986bba4
Compare
and for pluggability as well Signed-off-by: Antonio Murdaca <runcom@redhat.com>
Signed-off-by: Antonio Murdaca <runcom@redhat.com>
Docker is already specified in the pkg name Signed-off-by: Antonio Murdaca <runcom@redhat.com>
|
The last commit is the most important to me, before going to start vendoring containers/image in other tools (such as rkt) i'd really want to polish the API |
| type Client interface { | ||
| MakeRequest(string, string, map[string][]string, io.Reader, bool) (*http.Response, error) | ||
| } | ||
|
|
There was a problem hiding this comment.
Why export this at all? Do you expect any external users? If this is only for testing, we can make that interface private, and hide its uses from clients (see below).
There was a problem hiding this comment.
I think letting users specify a custom client at will is valuable
|
Full ACK on dropping the |
|
so the general stuff going on here is to have an extension point for people who want to provide their own client implementation - not just for our internal testing (it's clear we will gain something from this in tests but it's just a consequence) |
Like what? I genuinely don’t know why one would want to replace HTTP by something else, or to completely redo the ping and authorization lookup mechanism. Also, note that this would not help with testing I can, sort of, imagine a caller wanting to add the same HTTP timeouts as any other HTTP code in their application uses, or perhaps the same HTTP request/response logging that is used in the rest of the application. But why would a bona-fide user want to replace HTTP by something else, completely reimplement the authentication code, and so on? There may be something possibly useful to expose to the clients but the current |
|
so probably I'm over engineering but in Docker itself is a separate package with a given interface - probably we don't need this then as it is now |
|
What do you mean in particular? Looking at https://github.com/docker/distribution/blob/master/registry/client/repository.go#L65 , it is parametrized with a (Arguably |
I was referring to docker/docker and docker/engine-api but that's clearly another type of client |
|
superseded by #44 |
2 participants
@mtrmac pls see individual commits - the second commit just adds 1 test but if design is ok I'll continue adding them here