-
Notifications
You must be signed in to change notification settings - Fork 549
Conversation
@johnflavin I removed the event tests you introduced around here 06ef7c8 since they keep breaking in CI. Any idea on why they're flaking? |
Codecov Report
@@ Coverage Diff @@
## master #749 +/- ##
============================================
+ Coverage 63.68% 64.75% +1.07%
- Complexity 629 663 +34
============================================
Files 153 158 +5
Lines 2883 2931 +48
Branches 335 337 +2
============================================
+ Hits 1836 1898 +62
+ Misses 899 885 -14
Partials 148 148 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few other comments:
-
please put the GCR related stuff in it's own package, like
com.spotify.docker.client.gcr
-
seems like the only thing in GoogleContainerRegistryAuthSupplier.java actually related to GCR is the "refresher" that calls a command prior to reading the docker config file. Could this be abstracted a bit, so that someone could pass in any sort of type that did a "refresh" before reading the config file? Seems like if you allowed the user to pass in any
Supplier<Void>
orRunnable
essentially - a no-arg method with no return - then this class could be used for lots of other non-GCR purposes too
private Map<String, Object> headers = new HashMap<>(); | ||
private boolean registryAuthCalled; | ||
private boolean registryAuthCSupplierCalled; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo on the name? should this be registryAuthSupplierCalled
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
variable removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
variable removed
return this; | ||
} | ||
|
||
public DefaultDockerClient build() { | ||
if (registryAuthCalled && registryAuthCSupplierCalled) { | ||
throw new RuntimeException("LOGIC ERROR: DefaultDockerClient does not support being built " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IllegalStateException might be a better fit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also I think you could avoid the need for these two booleans by moving this check into the registryAuth(RegistryAuth)
and registryAuthSupplier(RegistryAuthSupplier)
methods - you can just throw an exception of this.registryAuthSupplier
is already set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
/** | ||
* Created by dmorhovich on 5/15/17. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer if this had at least a one line summary of what the class did - we don't need to have authors and dates in the file headers as we get that from Git.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, that's an intellij thing, deleted
private static final ObjectMapper MAPPER = ObjectMapperProvider.objectMapper(); | ||
|
||
|
||
public RegistryAuth fromComfig(Path configPath, String serverAddress) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: fromConfig
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
public class GCloudProcess { | ||
|
||
public Process runGcloudDocker() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there is no state, this should probably be a static method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs to be virtual so I can mock it via mockito
@@ -102,7 +96,8 @@ public final String toString() { | |||
*/ | |||
@SuppressWarnings("unused") | |||
public static Builder fromDockerConfig() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make this @Deprecated
if we would like for people to stop using it and to use the RegistryAuthSupplier instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@Override | ||
public RegistryAuth authFor(String imageName) throws DockerException { | ||
try { | ||
String registryName = "https://" + imageName.split("/")[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what would happen if I configure my DefaultDockerClient to use GoogleContainerRegistryAuthSupplier but then the client tries to pull an image from a non-GCR source? This doesn't seem to check to see if the image looks to be gcr.io related.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be needed, but I'd argue to make this a separate story/pr since it's adding a feature to this workflow.
* -/-/- | ||
*/ | ||
|
||
package com.spotify.docker.client.messages; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the wrong package for this class - messages
is supposed to be POJOs related to messages returned from/sent to the API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved
* -/-/- | ||
*/ | ||
|
||
package com.spotify.docker.client.messages; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please put in a different package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
* -/-/- | ||
*/ | ||
|
||
package com.spotify.docker.client.messages; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please put in a different package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Adds support for RegistryAuthSupplier. Client code can consume this using DefaultDockerClient builder. Provides support for authenticating against GCR to allow pushes/pulls from GCR.
if (isNullOrEmpty(serverAddress)) { | ||
final Iterator<String> servers = authJson.fieldNames(); | ||
if (servers.hasNext()) { | ||
serverAddress = servers.next(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will return the first credential in the file. We need the one, if any, that equals serverAddress
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's test this method with a test fixture/file with multiple serverAddresses.
public class GCloudProcess { | ||
|
||
public Process runGcloudDocker() throws IOException { | ||
return Runtime.getRuntime().exec("gcloud docker -a"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be better to run this like gcloud --account <account name> docker ...
so that the user can specify which account the credentials come from. Otherwise this is forcing that the "active account" in the user's/host's gcloud installation is the one to use for docker credentials. I think it would be worth decoupling those.
return authBuilder; | ||
} | ||
|
||
public Path defaultConfigPath() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be static.
} | ||
|
||
return request( | ||
GET, | ||
InputStream.class, | ||
resource, | ||
resource.request(APPLICATION_JSON_TYPE).header("X-Registry-Auth", authHeader(registryAuth)) | ||
resource.request(APPLICATION_JSON_TYPE).header("X-Registry-Auth", authHeader( | ||
registryAuthSupplier.authFor(images[0]))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This uses auth for first image. Can you export/save images from different registries at the same time? I couldn't find any docs for this.
} | ||
|
||
public Builder registryAuthSupplier(final RegistryAuthSupplier registryAuthSupplier) { | ||
if (this.registryAuthSupplier != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete these three lines to allow calling this method twice?
this.registryAuth = registryAuth; | ||
this.registryAuthSupplier = new NoOpRegistryAuthSupplier(registryAuth); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line makes calling this method twice throw which might not be obvious to caller if it doesn't know about the implementation here.
} | ||
|
||
@Test | ||
public void authForReturnsRegisteryAuthThatMatchesRegisteryName() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo in Registery
|
||
|
||
@Test | ||
public void refreshShellsOutToGCloudCli() throws IOException, InterruptedException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be ok not having this test :)
GoogleContainerRegistryCredRefresher googleContainerRegistryCredRefresher = | ||
new GoogleContainerRegistryCredRefresher(gcloudProcess); | ||
|
||
googleContainerRegistryCredRefresher.refresh();; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra semicolon
private static final ObjectMapper MAPPER = ObjectMapperProvider.objectMapper(); | ||
|
||
|
||
public RegistryAuth fromComfig(Path configPath, String serverAddress) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo in Comfig
refactor this test to use a MockWebServer rather than mocking the Jersey/jax-rs Client interface used by the DefaultDockerClient. Using a (Mockito) mock Client is problematic for a test since it has such a rich and fluent interface - the test needs to know exactly which methods are called on the Client. Ultimately, this is not what a unit test of DefaultDockerClient should care about - a good test would only care about what HTTP requests it sends to the Docker Remote API and how it behaves when it gets certain HTTP responses back. The MockWebServer allows us to control the HTTP responses and capture the HTTP requests much easier than using a Mockito mock of the particular HTTP client library used by DefaultDockerClient. With this approach, it would be possible to one day change DefaultDockerClient to use a different HTTP client library without having to change one line of this test. The same cannot be said of using Mockito. This change was initially driven by a desire to make PR #749 easier to test, to check that it sends expected authentication headers, but I think it will be useful and help improve testing in docker-client across the board. https://github.com/square/okhttp/tree/master/mockwebserver
refactor this test to use a MockWebServer rather than mocking the Jersey/jax-rs Client interface used by the DefaultDockerClient. Using a (Mockito) mock Client is problematic for a test since it has such a rich and fluent interface - the test needs to know exactly which methods are called on the Client. Ultimately, this is not what a unit test of DefaultDockerClient should care about - a good test would only care about what HTTP requests it sends to the Docker Remote API and how it behaves when it gets certain HTTP responses back. The MockWebServer allows us to control the HTTP responses and capture the HTTP requests much easier than using a Mockito mock of the particular HTTP client library used by DefaultDockerClient. With this approach, it would be possible to one day change DefaultDockerClient to use a different HTTP client library without having to change one line of this test. The same cannot be said of using Mockito. This change was initially driven by a desire to make PR #749 easier to test, to check that it sends expected authentication headers, but I think it will be useful and help improve testing in docker-client across the board. https://github.com/square/okhttp/tree/master/mockwebserver
I think that my test refactoring in #753 will make it much simpler to test what auth headers are sent to the docker API by this change |
I don't remember seeing the tests from #549 failing on travis. They do fail on my local machine sometimes due to the clock on docker's vm being out of sync with the clock on the host machine. Can you link to a travis build where they failed? |
@johnflavin I think this is an example of a failure, for a separate PR: https://travis-ci.org/spotify/docker-client/jobs/234609588. I don't have any data to support this but it seems to have started failing a lot more often just recently, but then again I've only been trying to get some PRs of my own merged recently so I may just not have noticed. @mavenraven I think it would make sense to pull the commits about CircleCI and the flaky test into a separate PR(s); that way we could merge them and benefit from them earlier without having to wait for this PR to be finished. |
continued in #759 |
Note that the integration test I added currently isn't running in CI.