Add core 'ports' functionality to docker context exporter, fix cross-platform Dockerfile write inconsistency #438

TadCordle · 2018-06-22T14:11:46Z

After merging #432. Part of #383.

…_ports_config

coollog · 2018-06-22T14:34:12Z

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

@@ -193,4 +206,19 @@ static String joinAsJsonArray(List<String> items) {

    return resultString.toString();
  }
+
+  /**
+   * Builds a list of Dockerfile "EXPOSE" items.


In Dockerfile terminology, they are called "instructions".

coollog · 2018-06-22T14:36:03Z

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

+  static String makeExposeItems(List<String> exposedPorts) {
+    StringBuilder resultString = new StringBuilder();
+    for (String port : exposedPorts) {
+      resultString.append("EXPOSE ").append(port).append("\n");


nit: can be '\n'

Also, if you don't want the extra new line at the end of the list, you can do a join instead.

coollog · 2018-06-22T14:37:24Z

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

+   * @return a string containing an EXPOSE command for each of the entries
+   */
+  @VisibleForTesting
+  static String makeExposeItems(List<String> exposedPorts) {


Looks like I messed up the ordering for joinAsJsonArray as well, but these static methods should be moved up top.

coollog · 2018-06-22T14:37:47Z

jib-core/src/main/resources/DockerfileTemplate

@@ -4,5 +4,5 @@ COPY libs @@DEPENDENCIES_PATH_ON_IMAGE@@
 COPY resources @@RESOURCES_PATH_ON_IMAGE@@
 COPY classes @@CLASSES_PATH_ON_IMAGE@@

-ENTRYPOINT @@ENTRYPOINT@@
+@@EXPOSED_PORTS@@ENTRYPOINT @@ENTRYPOINT@@


Should leave some new lines in between

I did that so there are no stray newlines when it gets replaced.

Yea, the stray newlines should be an implementation fix rather than on the template itself or it decreases the readability of the template.

Also, a nit, but perhaps EXPOSE_INSTRUCTIONS is a more applicable naming of the template section?

coollog · 2018-06-22T14:40:11Z

jib-core/src/test/resources/sampleDockerfile

@@ -4,5 +4,7 @@ COPY libs /app/libs/
 COPY resources /app/resources/
 COPY classes /app/classes/

+EXPOSE 1000
+EXPOSE 2000-2010


Is this a valid instruction? If not, we might want to reconsider supporting this type of syntax in our configuration, and try to keep it as similar to Dockerfile EXPOSE as possible.

What do you think about supporting UDP?

That sounds like a useful thing and probably wouldn't be too hard to add.

…b into port_docker_context

coollog · 2018-06-22T19:39:13Z

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

+  @VisibleForTesting
+  static String joinAsJsonArray(List<String> items) {
+    StringBuilder resultString = new StringBuilder("[");
+    boolean firstComponent = true;


Doing a join would avoid this first logic:

String.join("\n", exposedPorts.stream().map(port -> "EXPOSE " + port).collect(Collectors.toList()));

…t_docker_context

Needed to fix platform specific file stuff, would like a re-review please

coollog · 2018-06-22T21:00:45Z

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

@@ -84,7 +84,8 @@ static String joinAsJsonArray(List<String> items) {
  @VisibleForTesting
  static String makeExposeItems(List<String> exposedPorts) {
    return String.join(
-        "\n", exposedPorts.stream().map(port -> "EXPOSE " + port).collect(Collectors.toList()));
+        System.getProperty("line.separator"),


That's interesting. I'm wondering why this would need this platform-specific separator when the template's lines are separated by just LF?

Also, was the error with Docker not accepting non CRLF on windows?

I didn't look that deeply into it. The error was from the DockerContextGenerator unit test and said that the sample and the generated docker file lengths differed by 1 on windows, so I assumed it was using "\r\n" as separators and was missing a "\r".

Hmm that might mean the test should be fixed. I think the docker context generated should be platform independent so that a user generating on linux or windows would produce the same context for the same project.

coollog

I think we should still figure out why it was not working with the resource file though.

coollog · 2018-06-25T14:04:14Z

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

+   * @return a string containing an EXPOSE instruction for each of the entries
+   */
+  @VisibleForTesting
+  static String makeExposeItems(List<String> exposedPorts) {


makeExposeInstructions?

coollog · 2018-06-25T15:04:51Z

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

@@ -47,6 +46,18 @@
 */
 public class DockerContextGenerator {

+  /** The template to generate the Dockerfile from. */
+  private static final String DOCKERFILE_TEMPLATE =


If we're changing to just building the string in the code, we can just use stringbuilder and append the parts rather than doing the template replacement.

TadCordle · 2018-06-25T15:08:55Z

I think we should still figure out why it was not working with the resource file though.

Yeah I'm just trying things right now.

This reverts commit 1a71919.

This reverts commit c8dc74d.

TadCordle · 2018-06-25T19:30:24Z

@coollog @loosebazooka I think this is review-able, finally.

I got it working with the DockerfileTemplate in this commit, but ended up removing it anyway for efficiency.

coollog · 2018-06-25T19:38:34Z

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

+   */
+  @VisibleForTesting
+  static String joinAsJsonArray(List<String> items) {
+    return "["


Oh for this we could actually use ObjectMapper from the JSON library we use (I forgot about it when implementing this). It will automatically generate the JSON string from a list of strings.

You could do like

Blobs.writeToString(JsonTemplateMapper.toBlob(thelist))

I think it needs a ListOfJsonTemplate, not a list of strings?

Oh right, maybe just use new ObjectMapper().writeValueAsString directly.

coollog · 2018-06-25T19:42:30Z

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

-
-  /**
-   * Formats a list for the Dockerfile's ENTRYPOINT or CMD.
+   * Makes the contents of a {@code Dockerfile} using configuration data.


Since we're putting this in code, would be good to have in the javadoc an example of what it would look like.

coollog · 2018-06-25T20:44:14Z

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

+        + sourceFilesConfiguration.getResourcesPathOnImage()
+        + "\nCOPY classes "
+        + sourceFilesConfiguration.getClassesPathOnImage()
+        + "\n\n"


Oh wait, wouldn't this add 3 newlines if there weren't any exposed ports?

coollog · 2018-06-25T20:45:48Z

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

+   * @return a string containing an EXPOSE instruction for each of the entries
+   */
+  @VisibleForTesting
+  static String makeExposeInstructions(List<String> exposedPorts) {


This could also be moved to makeDockerfile and change that to append to a StringBuilder as an alternative.

…t_docker_context

TadCordle added 8 commits June 20, 2018 16:20

Add core changes for exposed port configuration

0b82cf6

Update integration tests

dd44f8b

Javadoc

fcfa21a

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

39c34a4

…_ports_config

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

ac8d674

…_ports_config

Feedback

218bce1

Add core exportPorts functionality to docker context exporter

5e68c1b

Remove EmptyStruct

d60d7eb

TadCordle requested review from loosebazooka and coollog June 22, 2018 14:11

TadCordle changed the title ~~Port docker context~~ Add core 'ports' functionality to docker context exporter Jun 22, 2018

This was referenced Jun 22, 2018

Add 'ports' configuration to gradle plugin #439

Merged

Add 'ports' configuration to maven plugin #440

Merged

coollog reviewed Jun 22, 2018

View reviewed changes

TadCordle added 9 commits June 22, 2018 11:42

Feedback

6b7fba5

Add support for udp

fefd342

Use builders and expand imports

7fa64bb

i m p o r t s

a71e7bb

Merge branch 'add_ports_config' of github.com:GoogleContainerTools/ji…

519d720

…b into port_docker_context

Feedback/protocol test

901c7d8

Use regex for splitting

cc25cd4

Stray newline stuff

7c6f7f1

Merge branch 'add_ports_config' of github.com:GoogleContainerTools/ji…

d2ab9d8

…b into port_docker_context

coollog reviewed Jun 22, 2018

View reviewed changes

TadCordle added 2 commits June 22, 2018 16:22

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

c13c6c2

…t_docker_context

?

bfec9bd

coollog previously approved these changes Jun 22, 2018

View reviewed changes

??

fb2fc0b

coollog reviewed Jun 22, 2018

View reviewed changes

Try generating dockerfile from string instead of template file

c8dc74d

coollog reviewed Jun 25, 2018

View reviewed changes

Try specifying carriage return

1a71919

TadCordle added the PR: Not Ready label Jun 25, 2018

TadCordle added 9 commits June 25, 2018 11:38

Revert "Try specifying carriage return"

ddf8336

This reverts commit 1a71919.

Revert "Try generating dockerfile from string instead of template file"

aab244a

This reverts commit c8dc74d.

testing

964d85a

Try another thing

9185594

hm

da15400

maybe

165a0d3

Try reading/writing line by line

213d32c

Consolidate joinAsJsonArray

8036cb3

Remove template, cleanup

0324e2a

TadCordle removed the PR: Not Ready label Jun 25, 2018

TadCordle changed the title ~~Add core 'ports' functionality to docker context exporter~~ Add core 'ports' functionality to docker context exporter, fix cross-platform Dockerfile write inconsistency Jun 25, 2018

coollog reviewed Jun 25, 2018

View reviewed changes

TadCordle added 3 commits June 25, 2018 16:05

Add javadoc example for makeDockerfile()

a0aed3d

Use ObjectMapper

0c550c8

Remove one line function

5c19ef5

coollog approved these changes Jun 25, 2018

View reviewed changes

coollog reviewed Jun 25, 2018

View reviewed changes

TadCordle added 2 commits June 25, 2018 16:54

Move makeExposeInstructions to makeDockerfile

b68aebb

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

b9bd25a

…t_docker_context

coollog approved these changes Jun 25, 2018

View reviewed changes

TadCordle merged commit 149bfeb into master Jun 25, 2018

TadCordle deleted the port_docker_context branch June 25, 2018 21:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add core 'ports' functionality to docker context exporter, fix cross-platform Dockerfile write inconsistency #438

Add core 'ports' functionality to docker context exporter, fix cross-platform Dockerfile write inconsistency #438

TadCordle commented Jun 22, 2018 •

edited

Loading

coollog Jun 22, 2018

coollog Jun 22, 2018

coollog Jun 22, 2018

coollog Jun 22, 2018

coollog Jun 22, 2018

TadCordle Jun 22, 2018

coollog Jun 22, 2018

coollog Jun 22, 2018

coollog Jun 22, 2018

TadCordle Jun 22, 2018

coollog Jun 22, 2018

coollog Jun 22, 2018

TadCordle Jun 22, 2018

coollog Jun 22, 2018

coollog Jun 22, 2018

TadCordle Jun 22, 2018 •

edited

Loading

coollog Jun 22, 2018

coollog left a comment

coollog Jun 25, 2018

coollog Jun 25, 2018

TadCordle commented Jun 25, 2018 •

edited

Loading

TadCordle commented Jun 25, 2018

coollog Jun 25, 2018

TadCordle Jun 25, 2018

coollog Jun 25, 2018

coollog Jun 25, 2018

coollog Jun 25, 2018

coollog Jun 25, 2018

Add core 'ports' functionality to docker context exporter, fix cross-platform Dockerfile write inconsistency #438

Add core 'ports' functionality to docker context exporter, fix cross-platform Dockerfile write inconsistency #438

Conversation

TadCordle commented Jun 22, 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

Choose a reason for hiding this comment

TadCordle Jun 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coollog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TadCordle commented Jun 25, 2018 • edited Loading

TadCordle commented Jun 25, 2018

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 Jun 22, 2018 •

edited

Loading

TadCordle Jun 22, 2018 •

edited

Loading

TadCordle commented Jun 25, 2018 •

edited

Loading