Added 'contextDir' configuration option. Used to specify Docker build context #1189

rohanKanojia · 2019-04-06T19:01:18Z

This is the continuation of #995

rohanKanojia · 2019-04-06T19:03:43Z

This is still wip, I haven't tested it properly and haven't updated docs yet.

… context (if Dockerfile located externally to build context) Signed-off-by: Andrey Osipov <[email protected]>

rhuss

Except that the Docker file should not be copied into the contextDir but in a generated directory below target/ where we build up the tar archive for the Docker daemon, the PR looks good.

See my comments, too.

rhuss · 2019-04-07T06:38:26Z

doc/changelog.md

  - Add support for auto-pulling multiple base image for multi stage builds (#1057)
  - Fix usage of credential helper that do not support 'version' command (#1159)
+
+Please note that `dockerFileDir` is now deprecated in favor of `contextDir` and it would be removed in 1.0.0.


... will be ..

rhuss · 2019-04-07T06:39:19Z

doc/changelog.md

  - Add support for auto-pulling multiple base image for multi stage builds (#1057)
  - Fix usage of credential helper that do not support 'version' command (#1159)
+
+Please note that `dockerFileDir` is now deprecated in favor of `contextDir` and it would be removed in 1.0.0.
+It's still supported in this release but users are suggested to migrate to `contextDir` instead.


Maybe state also the difference, namely that contextDir allows absolute paths to Dockerfile with dockerFile while dockerFileDir does not.

rhuss · 2019-04-07T06:40:03Z

samples/dockerfile/pom.xml

@@ -59,6 +59,7 @@
              <build>
                <!-- filter>@</filter-->
                <dockerFileDir>${project.basedir}/src/main/docker</dockerFileDir>


Please remove dockerFileDir in the sample as I guess its more confusing than helpful.

rhuss · 2019-04-07T06:40:58Z

src/main/asciidoc/inc/build/_overview.adoc

@@ -11,9 +11,10 @@ When using this mode, the Dockerfile is created on the fly with all instructions
 .External Dockerfile or Docker archive
 Alternatively an external Dockerfile template or Docker archive can be used. This mode is switched on by using one of these three configuration options within

-* *dockerFileDir* specifies a directory containing a Dockerfile that will be used to create the image. The name of the Dockerfile is `Dockerfile` by default but can be also set with the option `dockerFile` (see below).
-* *dockerFile* specifies a specific Dockerfile path. The Docker build context directory is set to `dockerFileDir` if given. If not the directory by default is the directory in which the Dockerfile is stored.
+* *dockerFileDir* *(This option is deprecated, please use contextDir instead)* specifies a directory containing a Dockerfile that will be used to create the image. The name of the Dockerfile is `Dockerfile` by default but can be also set with the option `dockerFile` (see below).


just use: (deprecated, use contextDir)

that's could enough ;-)

rhuss · 2019-04-07T06:41:41Z

src/main/asciidoc/inc/build/_overview.adoc

 * *dockerArchive* specifies a previously saved image archive to load directly. Such a tar archive can be created with `docker save`. If a `dockerArchive` is provided, no `dockerFile` or `dockerFileDir` must be given.
+* *contextDir* specifies docker build context if an external dockerfile is located outside of Docker build context. If not specified, Dockerfile's parent directory is used as build context.


exchange position of contextDir and dockerFileDir in list.

rhuss · 2019-04-07T08:00:45Z

src/main/java/io/fabric8/maven/docker/assembly/DockerAssemblyManager.java

@@ -130,7 +130,9 @@ public File createDockerTarArchive(String imageName, final MojoParameters params
                archiveCustomizers.add(new ArchiverCustomizer() {
                    @Override
                    public TarArchiver customize(TarArchiver archiver) throws IOException {
-                        DefaultFileSet fileSet = DefaultFileSet.fileSet(dockerFile.getParentFile());
+                        DefaultFileSet fileSet = DefaultFileSet.fileSet(buildConfig.getContextDir() != null


According to your change below, buildConfig.getContextDir() can never be null.

rhuss · 2019-04-07T08:03:31Z

src/main/java/io/fabric8/maven/docker/config/BuildImageConfiguration.java

-                    throw new IllegalArgumentException("<dockerFile> can not be absolute path if <dockerFileDir> also set.");
+                if(contextDir != null) {
+                    if(dockerFileDir != null) {
+                        log.warn("using contextDir as it has more precedence for Dockerfile");


Precedence over what ? Suggestion: "Both contextDir (%s) and deprecated dockerFileDir (%s) are configured. Using contextDir." (with inserting the concrete values)

rhuss · 2019-04-07T08:29:37Z

src/main/java/io/fabric8/maven/docker/config/BuildImageConfiguration.java

+
+                    if (dFile.isAbsolute()) {
+                        try {
+                            Files.copy(dFile.toPath(), Paths.get(contextDir));


I don't think that the dockerfile should be copied int the contextDir as this is typically a source code controlled directory. Intead it should be copied into the build dir where the context is copied to for creating the tar archive to be send to the Docker daemon.

Could you please tell me where are you creating tar archive? Is it here:

docker-maven-plugin/src/main/java/io/fabric8/maven/docker/assembly/DockerAssemblyManager.java

Lines 130 to 140 in dc82c82

archiveCustomizers.add(new ArchiverCustomizer() {

@Override

public TarArchiver customize(TarArchiver archiver) throws IOException {

DefaultFileSet fileSet = DefaultFileSet.fileSet(dockerFile.getParentFile());

addDockerIncludesExcludesIfPresent(fileSet, params);

// Exclude non-interpolated dockerfile from source tree

// Interpolated Dockerfile is already added as it was created into the output directory when

// using dir dir mode

excludeDockerfile(fileSet, dockerFile);

// If the content is added as archive, then we need to add the Dockerfile from the builddir

It's here where the Dockerfile is copied to its target directory:

docker-maven-plugin/src/main/java/io/fabric8/maven/docker/assembly/DockerAssemblyManager.java

Lines 196 to 202 in dec36f1

private void interpolateDockerfile(File dockerFile, BuildDirs params, FixedStringSearchInterpolator interpolator) throws IOException {

File targetDockerfile = new File(params.getOutputDirectory(), dockerFile.getName());

String dockerFileInterpolated = DockerFileUtil.interpolate(dockerFile, interpolator);

try (Writer writer = new FileWriter(targetDockerfile)) {

IOUtils.write(dockerFileInterpolated, writer);

}

}

okay, thanks

to bo honest, I am still confused. Aren't we copying Dockerfile here already into build directory? Could you please tell me also where is dockerFileDir/contextDir getting copied? do we need to copy Dockerfile just inside build or inside the contextDir?

hey, we were not done with this comment? Is it okay to merge it without addressing it?

src/main/java/io/fabric8/maven/docker/config/BuildImageConfiguration.java

rhuss

cool, thanks ! Looks good to me.

rohanKanojia added the WIP Work in Progress label Apr 6, 2019

Added 'contextDir' configuration option. Used to specify Docker build…

f54795d

… context (if Dockerfile located externally to build context) Signed-off-by: Andrey Osipov <[email protected]>

rohanKanojia removed the WIP Work in Progress label Apr 7, 2019

rohanKanojia force-pushed the pullReq995 branch from 6109c27 to b7d93e3 Compare April 7, 2019 06:23

rohanKanojia requested a review from rhuss April 7, 2019 06:26

rhuss requested changes Apr 7, 2019

View reviewed changes

rohanKanojia added the changes requested label Apr 7, 2019

rohanKanojia force-pushed the pullReq995 branch 2 times, most recently from df9c7e7 to cdc9c5c Compare April 7, 2019 10:38

Refactor BuildImageConfiguration in order to handle contextDir

c1e0d62

rohanKanojia force-pushed the pullReq995 branch from cdc9c5c to c1e0d62 Compare April 7, 2019 19:10

rhuss approved these changes Apr 7, 2019

View reviewed changes

rhuss merged commit 2ff5bd4 into fabric8io:master Apr 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added 'contextDir' configuration option. Used to specify Docker build context #1189

Added 'contextDir' configuration option. Used to specify Docker build context #1189

rohanKanojia commented Apr 6, 2019 •

edited

Loading

rohanKanojia commented Apr 6, 2019

rhuss left a comment

rhuss Apr 7, 2019

rhuss Apr 7, 2019

rhuss Apr 7, 2019

rhuss Apr 7, 2019

rhuss Apr 7, 2019

rhuss Apr 7, 2019

rhuss Apr 7, 2019

rhuss Apr 7, 2019

rohanKanojia Apr 7, 2019

rhuss Apr 7, 2019

rohanKanojia Apr 7, 2019

rohanKanojia Apr 7, 2019

rohanKanojia Apr 7, 2019

rhuss left a comment

		* dockerArchive specifies a previously saved image archive to load directly. Such a tar archive can be created with `docker save`. If a `dockerArchive` is provided, no `dockerFile` or `dockerFileDir` must be given.
		* contextDir specifies docker build context if an external dockerfile is located outside of Docker build context. If not specified, Dockerfile's parent directory is used as build context.

	archiveCustomizers.add(new ArchiverCustomizer() {
	@Override
	public TarArchiver customize(TarArchiver archiver) throws IOException {
	DefaultFileSet fileSet = DefaultFileSet.fileSet(dockerFile.getParentFile());
	addDockerIncludesExcludesIfPresent(fileSet, params);
	// Exclude non-interpolated dockerfile from source tree
	// Interpolated Dockerfile is already added as it was created into the output directory when
	// using dir dir mode
	excludeDockerfile(fileSet, dockerFile);

	// If the content is added as archive, then we need to add the Dockerfile from the builddir

	private void interpolateDockerfile(File dockerFile, BuildDirs params, FixedStringSearchInterpolator interpolator) throws IOException {
	File targetDockerfile = new File(params.getOutputDirectory(), dockerFile.getName());
	String dockerFileInterpolated = DockerFileUtil.interpolate(dockerFile, interpolator);
	try (Writer writer = new FileWriter(targetDockerfile)) {
	IOUtils.write(dockerFileInterpolated, writer);
	}
	}

Added 'contextDir' configuration option. Used to specify Docker build context #1189

Added 'contextDir' configuration option. Used to specify Docker build context #1189

Conversation

rohanKanojia commented Apr 6, 2019 • edited Loading

rohanKanojia commented Apr 6, 2019

rhuss left a 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

rhuss left a comment

Choose a reason for hiding this comment

rohanKanojia commented Apr 6, 2019 •

edited

Loading