Add plugin configuration for environment variables #930

TadCordle · 2018-09-04T20:14:58Z

Fixes #890

This adds a new environment configuration parameter to the maven and gradle plugins to set environment variables on the container image.

Maven:

<configuration>
  <container>
    <environment>
      <key1>value1</key1>
      <key2>value2</key2>
    </environment>
  </container>
</configuration>

Gradle:

jib {
  container {
    environment = [key1:"value1", key2:"value2"]
  }
}

This PR also adds the corresponding "ENV" command to the docker context generator.

Also slipped skip into the changelog to fix #929

briandealwis · 2018-09-04T20:19:45Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/JavaDockerContextGenerator.java

@@ -267,14 +282,24 @@ String makeDockerfile() throws JsonProcessingException {
      dockerfile.append("\nEXPOSE ").append(port);
    }

-    boolean firstLabel = true;
+    boolean firstElement = true;
+    for (Entry<String, String> env : environment.entrySet()) {


Reusing booleans is a error prone. Maybe this code should use java.util.StringJoiner instead — it supports a prefix.

briandealwis · 2018-09-04T20:23:10Z

jib-maven-plugin/CHANGELOG.md

@@ -5,6 +5,9 @@ All notable changes to this project will be documented in this file.

 ### Added

+- `<skip>` configuration parameter to skip Jib execution in multi-module projects ([#865](https://github.com/GoogleContainerTools/jib/issues/865))


and add also settable via property jib.skip

briandealwis · 2018-09-04T20:24:48Z

.../src/integration-test/java/com/google/cloud/tools/jib/builder/BuildStepsIntegrationTest.java

+    Assert.assertThat(
+        dockerContainerConfig,
+        CoreMatchers.containsString(
+            "                \"var1=value1\",\n                \"var2=value2\"\n"));


It would be nice to match the environment part ("Env": [) too, if possible.

The issue is that there are other environment variables already in there, like PATH and SSL_CERT_FILE. I guess I could use regex to skip those or just match those variables in the string as well (I'm not sure if they change though)?

Actually I can just use docker inspect -f {{.Config.Env}} <image>.

coollog · 2018-09-04T20:59:20Z

.../src/integration-test/java/com/google/cloud/tools/jib/builder/BuildStepsIntegrationTest.java

@@ -226,4 +197,31 @@ private BuildConfiguration getBuildConfiguration(
        .setCreatedBy("jib-integration-test")
        .build();
  }
+
+  private void assertDockerInspect(String imageReference) throws IOException, InterruptedException {


This looks like it can be static?

coollog · 2018-09-04T21:01:33Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/JavaDockerContextGenerator.java

+  /**
+   * Sets the environment variables
+   *
+   * @param environment the environment variables


nit: map from environment variable name to value?

coollog · 2018-09-04T21:04:11Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/JavaDockerContextGenerator.java

+    for (Entry<String, String> env : environment.entrySet()) {
+      joiner.add(env.getKey() + "=" + objectMapper.writeValueAsString(env.getValue()));
+    }
+    if (environment.entrySet().size() > 0) {


nit: can just be environment.size()

coollog · 2018-09-04T21:06:07Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/JavaDockerContextGenerator.java

+      dockerfile.append(joiner.toString());
+    }
+
+    joiner = new StringJoiner(" \\\n      ", "\nLABEL ", "");


The logic for labels seems to be the same as the logic for environment - maybe condense to a static method?

coollog · 2018-09-04T21:08:16Z

jib-gradle-plugin/src/integration-test/resources/projects/simple/complex-build.gradle

@@ -34,6 +34,7 @@ jib {
        args = ['An argument.']
        mainClass = 'com.test.HelloWorld'
        jvmFlags = ['-Xms512m', '-Xdebug']
+        environment = [var1:'value1', var2:'value2']


nit: it might be clearer if the values were like env1/env2?

…-config

coollog · 2018-09-04T22:04:06Z

jib-gradle-plugin/src/integration-test/resources/projects/simple/complex-build.gradle

@@ -34,7 +34,7 @@ jib {
        args = ['An argument.']
        mainClass = 'com.test.HelloWorld'
        jvmFlags = ['-Xms512m', '-Xdebug']
-        environment = [var1:'value1', var2:'value2']
+        environment = [env1:'value1', env2:'value2']


Oh I was referring to value1 and value2 since it would make the assertEquals be something like \nenv1value\nenv2value\n rather than \nvalue1\nvalue2\n where value1 and value2 are not immediately clear where they should have come from.

coollog · 2018-09-04T22:05:27Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/JavaDockerContextGenerator.java

+   * @throws JsonProcessingException if getting the json string of a map value fails
+   */
+  private static String mapToDockerfileString(
+      Map<String, String> map, String command, ObjectMapper objectMapper)


ObjectMapper can prob just be eliminated and made into a constant.

coollog · 2018-09-04T22:06:15Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/JavaDockerContextGenerator.java

+   *
+   * <pre>{@code
+   * command key1="value1" \
+   *     key2="value2" \


nit: should we line up key1 and key2 since we're adding spaces anyways?

The command names are different lengths, and adding a variable number of spaces here would have harmed readability or required a weird extra parameter, so I don't think it's worth it, unless there's a function we can use to generate whitespace that I don't know about (that doesn't require another import). I think it's fine as is.

patflynn · 2018-09-05T00:59:12Z

It'd be really nice if these PRs had a bit more content in the description so it's really easy for reviewers to understand what the changes are without looking at code.

For instance if this PR actually changes plugin config params it should include examples of what it enables.

TadCordle added 2 commits September 4, 2018 16:10

Add plugin configuration for environment variables

741a6e6

Add skip to changelog

40e2935

TadCordle added this to the v0.9.11 milestone Sep 4, 2018

TadCordle requested review from briandealwis, loosebazooka, coollog and chanseokoh September 4, 2018 20:14

googlebot added the cla: yes label Sep 4, 2018

briandealwis reviewed Sep 4, 2018

View reviewed changes

Feedback

e7b522e

coollog reviewed Sep 4, 2018

View reviewed changes

TadCordle added 3 commits September 4, 2018 17:46

Feedback

20d3fbf

Feedback

fef041c

Merge branch 'master' of github.com:GoogleContainerTools/jib into env…

415a53f

…-config

coollog reviewed Sep 4, 2018

View reviewed changes

Feedback

ee9a99e

coollog approved these changes Sep 5, 2018

View reviewed changes

TadCordle merged commit 499c3d6 into master Sep 5, 2018

TadCordle deleted the env-config branch September 5, 2018 14:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add plugin configuration for environment variables #930

Add plugin configuration for environment variables #930

TadCordle commented Sep 4, 2018 •

edited

Loading

briandealwis Sep 4, 2018

briandealwis Sep 4, 2018

briandealwis Sep 4, 2018

TadCordle Sep 4, 2018

TadCordle Sep 4, 2018

coollog Sep 4, 2018

coollog Sep 4, 2018

coollog Sep 4, 2018

coollog Sep 4, 2018

coollog Sep 4, 2018

coollog Sep 4, 2018

coollog Sep 4, 2018

coollog Sep 4, 2018

TadCordle Sep 5, 2018

patflynn commented Sep 5, 2018

		@@ -5,6 +5,9 @@ All notable changes to this project will be documented in this file.

		### Added

		- `<skip>` configuration parameter to skip Jib execution in multi-module projects ([#865](https://github.com/GoogleContainerTools/jib/issues/865))

Add plugin configuration for environment variables #930

Add plugin configuration for environment variables #930

Conversation

TadCordle commented Sep 4, 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

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

patflynn commented Sep 5, 2018

TadCordle commented Sep 4, 2018 •

edited

Loading