Support relative paths in `volumes` statements in docker-compose.yaml #848

emetsger · 2017-08-23T20:34:53Z

Moves path resolution logic to a utility class
Includes tests
Adds mockito-core as a test dependency

Addresses #846

Signed-off-by: Elliot Metsger emetsger@jhu.edu

codecov · 2017-08-23T20:39:18Z

Codecov Report

Merging #848 into master will increase coverage by 0.33%.
The diff coverage is 74.19%.

@@             Coverage Diff              @@
##             master     #848      +/-   ##
============================================
+ Coverage     50.12%   50.46%   +0.33%     
- Complexity     1151     1180      +29     
============================================
  Files           134      137       +3     
  Lines          6951     7033      +82     
  Branches        916      935      +19     
============================================
+ Hits           3484     3549      +65     
- Misses         3164     3177      +13     
- Partials        303      307       +4

Impacted Files	Coverage Δ	Complexity Δ
...c/main/java/io/fabric8/maven/docker/StartMojo.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
.../io/fabric8/maven/docker/service/WatchService.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...ig/handler/compose/DockerComposeConfigHandler.java	`71.73% <100%> (+0.46%)`	`17 <0> (-1)`	⬇️
...a/io/fabric8/maven/docker/util/DockerPathUtil.java	`61.53% <61.53%> (ø)`	`3 <3> (?)`
...en/docker/config/handler/compose/ComposeUtils.java	`70% <70%> (ø)`	`4 <4> (?)`
...g/handler/compose/DockerComposeServiceWrapper.java	`53.76% <77.77%> (+0.69%)`	`52 <1> (ø)`	⬇️
...va/io/fabric8/maven/docker/service/RunService.java	`52.35% <80%> (+0.45%)`	`25 <0> (ø)`	⬇️
...o/fabric8/maven/docker/util/VolumeBindingUtil.java	`80.39% <80.39%> (ø)`	`22 <22> (?)`
... and 3 more

rhuss

Thanks a lot, the functionality looks good to me and makes sense.

I have still some minor requests, though:

would be cool if we can stick to jmockit as mocking lib.
To make the behaviour consistent I would like to add this functionality to the 'normal' bind volumes via the regular configuration.
Update of the documentation in src/main/asciidoc would be awesome.
Add a line changelog.md for the new feature.

thanks!

rhuss · 2017-08-24T08:29:32Z

src/test/java/io/fabric8/maven/docker/config/handler/compose/ComposeUtilsTest.java

+import java.io.File;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.mock;


would it be able to use jmockit as Mock framework (as we already use this and I'm not so inclined to use multiple mock frameworks in d-m-p)

You could take e.g. https://github.com/rhuss/docker-maven-plugin/blob/674e80fdf56d31fdd263366d53e7a5cf0dc064b3/src/test/java/io/fabric8/maven/docker/access/log/LogRequestorTest.java#L220 as a sample how expectations are setup in jmockit.

rhuss · 2017-08-24T08:46:25Z

src/main/java/io/fabric8/maven/docker/config/handler/compose/DockerComposeServiceWrapper.java

+     * @param volume the volume string from the docker-compose file
+     * @return the volume string, with any relative paths resolved as absolute paths
+     */
+    static String resolveRelativeVolumeBinding(File baseDir, String volume) {


It would be awesome if we could use the same kind of parsing for the regular configuration, too.

E.g. in

https://github.com/rhuss/docker-maven-plugin/blob/674e80fdf56d31fdd263366d53e7a5cf0dc064b3/src/main/java/io/fabric8/maven/docker/service/RunService.java#L258

https://github.com/rhuss/docker-maven-plugin/blob/674e80fdf56d31fdd263366d53e7a5cf0dc064b3/src/main/java/io/fabric8/maven/docker/service/RunService.java#L332

The challenge there is to get the basedir into the method (which must not have a reference to any Maven code). Needs probably given through top-down.

It appears that relative paths in volume binding strings can only appear in docker-compose.yaml files.

The Docker Run reference indicates that relative paths are not allowed in image VOLUMEs:

-v, --volume=[host-src:]container-dest[:]: Bind mount a volume.
The comma-delimited options are [rw|ro], [z|Z],
[[r]shared|[r]slave|[r]private], and [nocopy].
The 'host-src' is an absolute path or a name value.

And later in the same section:

The container-dest must always be an absolute path such as /src/docs. The host-src can either be an absolute path or a name value. If you supply an absolute path for the host-dir, Docker bind-mounts to the path you specify. If you supply a name, Docker creates a named volume by that name.

A name value must start with an alphanumeric character, followed by a-z0-9, _ (underscore), . (period) or - (hyphen). An absolute path starts with a / (forward slash).

For example, you can specify either /foo or foo for a host-src value. If you supply the /foo value, Docker creates a bind-mount. If you supply the foo specification, Docker creates a named volume.

Let me know if you don't agree, but it seems that relative paths only need to be accounted for when using a docker-compose.yml file.

In that case, the base directory used to resolve relative paths should be the one defined in the compose handler:

<external> <type>compose</type> <basedir>src/main/docker</basedir> <composeFile>docker-compose.yml</composeFile> </external>

Overall, I'm not sure of the wider applicability this code. What are your thoughts? Maybe I am missing something. I can certainly clean up the PR a bit, and have the resolution logic ready to be used by other classes as needed.

emetsger · 2017-08-24T19:32:06Z

Hi @rhuss,

Let me know if you think I've got all the bases covered. I tried to address each of your comments, but note my response here which may be hidden depending on how the GitHub UI renders the comment in your browser.

I think the major portions of the logic are public and have decent test coverage. If other portions of the code base want to support relative volume binding strings, the VolumeUtil class has two methods to support this.

Again, based on my reasoning above, I'm not sure if there are other portions of the codebase that can take advantage of this, but I just may not be understanding the code base, or your request. Let me know if there's anything else you need.

emetsger · 2017-08-29T01:06:17Z

@rhuss just a ping on this PR- many thanks for your help/direction!

rhuss · 2017-08-29T06:26:22Z

@emetsger Thanks, will have a look ASAP. hope for today, but work is killing me ...

* Moves path resolution logic to a utility class * Includes tests * Adds mockito-core as a test dependency Addresses fabric8io#846 Signed-off-by: Elliot Metsger <emetsger@jhu.edu>

Signed-off-by: Elliot Metsger <emetsger@jhu.edu>

…o use DockerPathUtil. Signed-off-by: Elliot Metsger <emetsger@jhu.edu>

… VolumeBindingUtil. Includes test updates. Signed-off-by: Elliot Metsger <emetsger@jhu.edu>

…asedir (fabric8io#846). Signed-off-by: Elliot Metsger <emetsger@jhu.edu>

emetsger · 2017-08-30T14:21:22Z

@rhuss fixed test failures, added some javadoc, rebased against master.

Signed-off-by: Elliot Metsger <emetsger@jhu.edu>

…resolve relative path references in docker compose files. Signed-off-by: Elliot Metsger <emetsger@jhu.edu>

rhuss · 2017-08-30T20:56:42Z

Thanks a lot !

@nicolaferraro could you have a brief look, I'm really so overloaded ;-( This is an awesome PR, I would like to get it merged asap (before my PTO next week)

nicolaferraro · 2017-08-31T08:02:35Z

Sure @rhuss, I can look at it this morning.

nicolaferraro

@emetsger this PR is really awesome!
You've implemented most of the suggestions expressed by Roland.

Btw, I also think that the behavior should be kept consistent with "bind". If you look at the volume documentation, mount points are allowed in the plugin config and users should be able to use relative paths also there.

Even if docker run does not support relative paths, the plugin has its own configuration schema and it can fill the gaps. The base dir in that case would be the project root imho.

I think that using relative paths in the "bind" configuration has real advantages.. e.g. you can create a container that supports hot reloading by sharing the "./target" directory..

Good stuff!

Hope you can do this fix (Roland has already suggested some starting points in previous comments).

nicolaferraro · 2017-08-31T11:51:48Z

src/main/java/io/fabric8/maven/docker/util/VolumeBindingUtil.java

+
+        // only perform resolution if the localPath begins with '~/' or './'
+
+        if (localPath.startsWith("./")) {


Maybe a better option would be using the File#getCanonicalPath method to resolve relative paths. So that also ../ is resolved correctly.

* docker-compose.yaml files may be resolved by relative parent references (e.g. "../path/to/docker-compose.yaml") * volume bind mounts may specifiy relative parent references (e.g. "../path/to/dir:/container/mount/point") * includes updated tests and comments Signed-off-by: Elliot Metsger <emetsger@jhu.edu>

emetsger · 2017-08-31T20:02:36Z

@nicolaferraro @rhuss ~~I think I've hit everything, please let me know if more updates are needed! Thanks for the review!~~

I think there are a couple more things that need to be done. For example, this PR doesn't handle volume bind strings that specify read permissions like /host_path:/container_path:ro, so I need to fix that up.

Also, I'm not fully convinced that my changes will work on the Windows platform. Is there someone who can test this PR on Windows? I can probably get my hands on a machine somewhere if that's too much trouble for you all.

nicolaferraro · 2017-09-01T08:40:30Z

@emetsger it would be good if you can test it on windows (and/or evaluate what's needed to be changed). Unfortunately (or fortunately 😄) I'm a lot far from any windows machine right now...

Signed-off-by: Elliot Metsger <emetsger@jhu.edu>

* Consolidates constants and test-related utility methods in PathTestUtil * Updates Javadoc and comments * Updates DockerFileUtilTest to be tolerant of line-ending differences (fails on Windows without these changes) Signed-off-by: Elliot Metsger <emetsger@jhu.edu>

emetsger · 2017-09-07T17:56:02Z

@nicolaferraro @rhuss OK! This PR has been tested on Windows; it does include a minor update to DockerFileUtilTest.java, an existing test that failed on the Windows platform due to end-of-line character differences. The access controls like ro and rw are also preserved.

Please feel free to squash the PR, and/or let me know if you need anything else!

nicolaferraro · 2017-09-08T10:13:01Z

Great @emetsger, I'll take another look later.

nicolaferraro · 2017-09-11T12:20:27Z

LGTM @emetsger. Great work!
Let's wait for a final check by @rhuss before merging (he may not be available this week).

rhuss · 2017-09-11T13:54:51Z

[merge]

rhuss · 2017-09-11T13:59:51Z

@emetsger thanks !

emetsger · 2017-09-11T14:01:07Z

@rhuss @nicolaferraro thank you folks for a great plugin! Happy to make a small contribution :)

rhuss requested changes Aug 24, 2017

View reviewed changes

emetsger force-pushed the volumes-846 branch from a1b20f0 to 3138321 Compare August 24, 2017 19:15

emetsger added 5 commits August 30, 2017 09:27

Support relative paths in volumes statements in docker-compose.yaml

b2e9238

* Moves path resolution logic to a utility class * Includes tests * Adds mockito-core as a test dependency Addresses fabric8io#846 Signed-off-by: Elliot Metsger <emetsger@jhu.edu>

Use JMockit; remove Mockito.

e962997

Signed-off-by: Elliot Metsger <emetsger@jhu.edu>

Add a public DockerPathUtil class (with tests). Update ComposeUtils t…

ed66e24

…o use DockerPathUtil. Signed-off-by: Elliot Metsger <emetsger@jhu.edu>

Move DockerComposeServiceWrapper.resolveRelativeVolumeBinding(...) to…

d6cd442

… VolumeBindingUtil. Includes test updates. Signed-off-by: Elliot Metsger <emetsger@jhu.edu>

Volume binding strings with relative paths are resolved against the b…

f5a988b

…asedir (fabric8io#846). Signed-off-by: Elliot Metsger <emetsger@jhu.edu>

emetsger force-pushed the volumes-846 branch 2 times, most recently from 9c6fb76 to 9cd4375 Compare August 30, 2017 14:08

emetsger added 3 commits August 30, 2017 15:05

Update changelog

5289775

Signed-off-by: Elliot Metsger <emetsger@jhu.edu>

Additional javadoc and more informative exception handling.

84e0bba

Signed-off-by: Elliot Metsger <emetsger@jhu.edu>

Update test configuration by providing a base directory necessary to …

bddc00e

…resolve relative path references in docker compose files. Signed-off-by: Elliot Metsger <emetsger@jhu.edu>

emetsger force-pushed the volumes-846 branch from 9cd4375 to bddc00e Compare August 30, 2017 19:05

nicolaferraro requested changes Aug 31, 2017

View reviewed changes

emetsger force-pushed the volumes-846 branch from bf97704 to 25eddf3 Compare September 1, 2017 13:50

emetsger added 2 commits September 6, 2017 10:25

Support relative paths in the <run> configuration.

51ca2dd

Signed-off-by: Elliot Metsger <emetsger@jhu.edu>

Support access control specifications in volume binding strings..

8552deb

Signed-off-by: Elliot Metsger <emetsger@jhu.edu>

emetsger force-pushed the volumes-846 branch from d7bf83c to f44c31b Compare September 6, 2017 14:26

emetsger force-pushed the volumes-846 branch from f44c31b to 406a690 Compare September 7, 2017 17:47

nicolaferraro approved these changes Sep 11, 2017

View reviewed changes

rhuss approved these changes Sep 11, 2017

View reviewed changes

fusesource-ci merged commit 802fa9b into fabric8io:master Sep 11, 2017

emetsger mentioned this pull request Sep 12, 2017

Support relative paths in docker-compose 'volumes:' statements #846

Closed

emetsger mentioned this pull request Nov 1, 2017

0.22 Release? #886

Closed

ockham mentioned this pull request Apr 19, 2021

wp-env: First exploration of a static docker-compose.yml WordPress/gutenberg#30974

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support relative paths in `volumes` statements in docker-compose.yaml #848

Support relative paths in `volumes` statements in docker-compose.yaml #848

emetsger commented Aug 23, 2017

codecov bot commented Aug 23, 2017 •

edited

Loading

rhuss left a comment

rhuss Aug 24, 2017

rhuss Aug 24, 2017

emetsger Aug 24, 2017

emetsger commented Aug 24, 2017

emetsger commented Aug 29, 2017

rhuss commented Aug 29, 2017

emetsger commented Aug 30, 2017

rhuss commented Aug 30, 2017

nicolaferraro commented Aug 31, 2017

nicolaferraro left a comment

nicolaferraro Aug 31, 2017

emetsger commented Aug 31, 2017 •

edited

Loading

nicolaferraro commented Sep 1, 2017

emetsger commented Sep 7, 2017 •

edited

Loading

nicolaferraro commented Sep 8, 2017

nicolaferraro commented Sep 11, 2017

rhuss commented Sep 11, 2017

rhuss commented Sep 11, 2017

emetsger commented Sep 11, 2017


		// only perform resolution if the localPath begins with '~/' or './'

		if (localPath.startsWith("./")) {

Support relative paths in volumes statements in docker-compose.yaml #848

Support relative paths in volumes statements in docker-compose.yaml #848

Conversation

emetsger commented Aug 23, 2017

codecov bot commented Aug 23, 2017 • edited Loading

Codecov Report

rhuss left a comment

Choose a reason for hiding this comment

rhuss Aug 24, 2017

Choose a reason for hiding this comment

rhuss Aug 24, 2017

Choose a reason for hiding this comment

emetsger Aug 24, 2017

Choose a reason for hiding this comment

emetsger commented Aug 24, 2017

emetsger commented Aug 29, 2017

rhuss commented Aug 29, 2017

emetsger commented Aug 30, 2017

rhuss commented Aug 30, 2017

nicolaferraro commented Aug 31, 2017

nicolaferraro left a comment

Choose a reason for hiding this comment

nicolaferraro Aug 31, 2017

Choose a reason for hiding this comment

emetsger commented Aug 31, 2017 • edited Loading

nicolaferraro commented Sep 1, 2017

emetsger commented Sep 7, 2017 • edited Loading

nicolaferraro commented Sep 8, 2017

nicolaferraro commented Sep 11, 2017

rhuss commented Sep 11, 2017

rhuss commented Sep 11, 2017

emetsger commented Sep 11, 2017

Support relative paths in `volumes` statements in docker-compose.yaml #848

Support relative paths in `volumes` statements in docker-compose.yaml #848

codecov bot commented Aug 23, 2017 •

edited

Loading

emetsger commented Aug 31, 2017 •

edited

Loading

emetsger commented Sep 7, 2017 •

edited

Loading