Allow configuration docker executable path and additional environment variables #1214

chanseokoh · 2018-11-02T20:21:52Z

#468 for Gradle. Branched off of #1173.

Allow configuration docker executable path and additional environment variables.

jibDockerBuild.dockerClient.executable = '/usr/bin/docker'
jibDockerBuild.dockerClient.environment = [KEY: 'value', DOCKER_HOST: '...']

jibDockerBuild {
  dockerClient {
    executable = ...
    environment = ...
  }
}

- added a protected DockerClient.getProcessBuilderFactory() method for test purposes - removed a no longer used defaultProcessBuilderFactory that had no enviromment variable configuration DockerClienttest: - added DockerClient tests BuilderDockerTask: - applied google code styling

removed unused methods merged dockerExecutable & dockerEnvironemnt to dockerClient

…ub.com/gintautassulskus/jib into i468-docker-client-env

googlebot · 2018-11-02T20:21:58Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2018-11-02T20:22:36Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

chanseokoh · 2018-11-02T20:26:16Z

jib-core/src/test/java/com/google/cloud/tools/jib/api/DockerDaemonImageTest.java

 ImageConfiguration imageConfiguration = dockerDaemonImage.toImageConfiguration();

 Assert.assertEquals(
 ImageReference.parse("docker/daemon/image").toString(),
 imageConfiguration.getImage().toString());
 Assert.assertEquals(0, imageConfiguration.getCredentialRetrievers().size());
- Assert.assertEquals(Paths.get("docker", "executable"), dockerDaemonImage.getDockerExecutable());


Removed, as this just tests the simple getter/setter and seemed not worth adding a getter for testing.

coollog · 2018-11-02T20:57:59Z

jib-core/src/main/java/com/google/cloud/tools/jib/docker/DockerClient.java

@@ -41,17 +44,22 @@
 * @return a new {@link DockerClient}
 */
 public static DockerClient newClient() {
- return new DockerClient(defaultProcessBuilderFactory(DEFAULT_DOCKER_CLIENT));
+ return newClient(null, null);


Probably just pass DEFAULT_DOCKER_CLIENT and Collections.emptyMap() so the parameters don't need to be nullable?

That works, but the downside is that BuildDockerTask will have to do

if (dockerExecutable != null && dockerEnvironment != null) { ... = DockerClient.newClient(dockerExecutable, dockerEnvironment); } else if (dockerExecutable == null && dockerEnvironment == null ) { ... = DockerClient.newClient(); } else { ... ??? }

or make internal defaults of DockerClient public, so that

... = DockerClient.newClient( dockerClient == null ? DockerClient.DEFAULT_DOCKER_CLIENT : dockerClient.toString(), dockerEnvironment == null ? ... : dockerEnvironment);

or define DockerBuildTask's own defaults, duplicating defaults in multiple places.

Hmm, maybe this warrants a simple Builder to make the construct clearer - using null to mean default where the default is not null feels a bit off

Good idea. Let me try a builder.

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/DockerClientParameters.java

coollog · 2018-11-02T21:01:04Z

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/DockerClientParameters.java

+ @Input
+ @Nullable
+ @Optional
+ public Map<String, String> getEnvironment() {


I think setEnvironment is missing

I noticed build.gradle works without it (don't know why), but do you think the setter should actually be there?

Hmm that's interesting. How does it know what the set.. @loosebazooka Is no setter a valid construct?

One thing I also noticed is that, JibExtension doesn't have a setter for useOnlyProjectCache or allowInsecureRegistries either. (And interestingly, the getters for them are not public but package-private.)

But perhaps il'll be just safer to add a setter. Who knows if older Gradle versions won't work if there is no setter. I'll add a setter.

I might just be remembering things wrong and perhaps a setter is only necessary if we need to accept a different type than what's declared on the getter.

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/GradleRawConfiguration.java

jib-core/src/main/java/com/google/cloud/tools/jib/docker/DockerClient.java

coollog · 2018-11-05T21:22:56Z

jib-core/src/main/java/com/google/cloud/tools/jib/docker/DockerClient.java

@@ -79,19 +95,22 @@ public DockerClient build() {
 @VisibleForTesting
 static Function<List<String>, ProcessBuilder> defaultProcessBuilderFactory(
 String dockerExecutable, Map<String, String> dockerEnvironment) {
+ Map<String, String> environmentCopy = new HashMap<>(dockerEnvironment);


What about just having this method take an ImmutableMap? I think we should generally favor ImmutableMap over copying a HashMap in our non-public API since ImmutableMap.copyOf doesn't necessarily always perform a copy.

I'm not using ImmutableMap.copyOf and I want to believe copyOf() does the operation safely (good to know this anyway), but I'll change the argument to ImmutableMap, as this is not part of the public API.

TadCordle · 2018-11-07T21:46:25Z

We should probably add a changelog entry for this. Also are we waiting until 0.10.1 before merging this so we have more time to add the maven side?

coollog · 2018-11-07T21:50:52Z

I think we can actually get this in for 0.10.0.

chanseokoh · 2018-11-07T21:52:57Z

Yeah, I think we can get this in for 0.10.0. We can do the Maven part later. I will update CHANGELOG in another PR very soon.

gintautassulskus and others added 13 commits October 21, 2018 01:23

added executable and environment parameters for jibDockerBuild

b1e2a79

reverted bnack to google code styling

f9d7448

fioxed method documentation

56e3a2f

fixed code styling

4fb9d8c

addressed ode style remarks

cd69bc0

removed unused methods merged dockerExecutable & dockerEnvironemnt to dockerClient

reversed unintentional changes

940a6ea

added VisibleFortesting annotation to DockerDaemonImage method

05ef33e

added dockerClient setter

124b255

WIP after merging branch 'jibDockerBuild-env-support' of https://gith…

d2bd7a4

…ub.com/gintautassulskus/jib into i468-docker-client-env

Allow settings docker executable and environment

07c52fa

Clean up

d55a378

Clean up

8b6fa35

googlebot added the cla: no label Nov 2, 2018

chanseokoh added cla: yes and removed cla: no labels Nov 2, 2018

chanseokoh commented Nov 2, 2018

View reviewed changes

coollog reviewed Nov 2, 2018

View reviewed changes

public DockerClientParameters

b78251c

googlebot added cla: no and removed cla: yes labels Nov 2, 2018

chanseokoh added cla: yes and removed cla: no labels Nov 2, 2018

GoogleContainerTools deleted a comment from googlebot Nov 2, 2018

Revert RawConfiguration

f25d68b

googlebot added cla: no and removed cla: yes labels Nov 2, 2018

googlebot added cla: no and removed cla: yes labels Nov 5, 2018

Add Javadoc

180c6cd

chanseokoh added cla: yes and removed cla: no labels Nov 5, 2018

GoogleContainerTools deleted a comment from googlebot Nov 5, 2018

Fix Javadoc

f6968d8

googlebot added cla: no and removed cla: yes labels Nov 5, 2018

chanseokoh added cla: yes and removed cla: no labels Nov 5, 2018

GoogleContainerTools deleted a comment from googlebot Nov 5, 2018

coollog approved these changes Nov 5, 2018

View reviewed changes

Use ImmutableMap in argument

b76e4f3

googlebot added cla: no and removed cla: yes labels Nov 5, 2018

GoogleContainerTools deleted a comment from googlebot Nov 5, 2018

chanseokoh added cla: yes and removed cla: no labels Nov 5, 2018

chanseokoh merged commit b4cce76 into master Nov 7, 2018

chanseokoh deleted the i468-docker-client-env branch November 7, 2018 21:53

This was referenced Nov 7, 2018

CHANGELOG for jibDockerBuild.dockerClient params #1223

Merged

added executable and environment parameters for jibDockerBuild #1173

Closed

This was referenced Jan 4, 2019

Add docker client executable/environment configuration to Maven #1393

Merged

Add docker executable/env configuration to READMEs or FAQ #1394

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuration docker executable path and additional environment variables #1214

Allow configuration docker executable path and additional environment variables #1214

chanseokoh commented Nov 2, 2018

googlebot commented Nov 2, 2018

googlebot commented Nov 2, 2018

chanseokoh Nov 2, 2018

coollog Nov 2, 2018

chanseokoh Nov 2, 2018 •

edited

Loading

coollog Nov 5, 2018

chanseokoh Nov 5, 2018

coollog Nov 2, 2018

chanseokoh Nov 2, 2018

coollog Nov 5, 2018

chanseokoh Nov 5, 2018

coollog Nov 5, 2018

coollog Nov 5, 2018

chanseokoh Nov 5, 2018

TadCordle commented Nov 7, 2018

coollog commented Nov 7, 2018

chanseokoh commented Nov 7, 2018

Allow configuration docker executable path and additional environment variables #1214

Allow configuration docker executable path and additional environment variables #1214

Conversation

chanseokoh commented Nov 2, 2018

googlebot commented Nov 2, 2018

googlebot commented Nov 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chanseokoh Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TadCordle commented Nov 7, 2018

coollog commented Nov 7, 2018

chanseokoh commented Nov 7, 2018

chanseokoh Nov 2, 2018 •

edited

Loading