You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Andrew Donald Kennedy <no...@github.com> on 2015/04/06 17:37:55 UTC
[jclouds-labs] Add template option to specify direct (unmapped) ports
(#162)
Allows container port to map directly to same port on host for e.g. public web servers on 80 or 443.
```Java
DockerTemplateOptions options = ...
options.directPorts(ImmutableList.of(80, 443));
```
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/162
-- Commit Summary --
* Add template option to specify direct (unmapped) ports
-- File Changes --
M docker/src/main/java/org/jclouds/docker/compute/options/DockerTemplateOptions.java (53)
M docker/src/main/java/org/jclouds/docker/compute/strategy/DockerComputeServiceAdapter.java (12)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/162.patch
https://github.com/jclouds/jclouds-labs/pull/162.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162
Re: [jclouds-labs] Add template option to specify direct (unmapped)
ports (#162)
Posted by Andrea Turli <no...@github.com>.
@grkvlt could you fix the checkstyle violation at
```DockerComputeServiceAdapter.java[22:8] (imports) UnusedImports: Unused import - java.util.Arrays.```
so to make the builder happier?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162#issuecomment-90406418
Re: [jclouds-labs] Add template option to specify direct (unmapped)
ports (#162)
Posted by Ignasi Barrera <no...@github.com>.
> .toString();
> }
>
> public static final DockerTemplateOptions NONE = new DockerTemplateOptions();
>
> public DockerTemplateOptions volumes(Map<String, String> volumes) {
> - this.volumes = Optional.<Map<String, String>> of(ImmutableMap.copyOf(volumes));
> + for (Map.Entry<String, String> entry : checkNotNull(volumes, "volumes").entrySet()) {
> + checkNotNull(entry.getKey(), "host paths must not be null");
> + checkNotNull(entry.getValue(), "container paths must not be null");
> + }
In general for the whole PR, are these checks really needed? Guava's immutable collections don't allow null values so checking the null input in the map and list options here looks redundant?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162/files#r27837812
Re: [jclouds-labs] Add template option to specify direct (unmapped)
ports (#162)
Posted by Andrea Turli <no...@github.com>.
I think 2 commits is ok. @demobox @nacx, thoughts ?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162#issuecomment-90970115
Re: [jclouds-labs] Add template option to specify direct (unmapped)
ports (#162)
Posted by Andrea Turli <no...@github.com>.
Closed #162.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162#event-276555587
Re: [jclouds-labs] Add template option to specify direct (unmapped)
ports (#162)
Posted by Andrew Donald Kennedy <no...@github.com>.
@andreaturli what do you mean?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162#issuecomment-91011138
Re: [jclouds-labs] Add template option to specify direct (unmapped)
ports (#162)
Posted by Andrea Turli <no...@github.com>.
Merged at [8d1404038dfe952b7fc](https://git1-us-west.apache.org/repos/asf?p=jclouds-labs.git;a=commit;h=8d140403)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162#issuecomment-91014842
Re: [jclouds-labs] Add template option to specify direct (unmapped)
ports (#162)
Posted by Andrea Turli <no...@github.com>.
@grkvlt the previous build failed because of an external error, so I asked for a rebuild in that way.
Merging the PR just now
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162#issuecomment-91012168
Re: [jclouds-labs] Add template option to specify direct (unmapped)
ports (#162)
Posted by Andrea Turli <no...@github.com>.
Lgtm, very minor comment.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162#issuecomment-90269185
Re: [jclouds-labs] Add template option to specify direct (unmapped)
ports (#162)
Posted by Andrew Donald Kennedy <no...@github.com>.
> @@ -136,6 +140,14 @@ public DockerComputeServiceAdapter(DockerApi api) {
> .publishAllPorts(true)
> .privileged(true);
>
> + if (templateOptions.getDirectPorts().isPresent()) {
Agree. Could have a `Map<Integer, Integer>` type here, instead? This would be mappings of host port to container port (this is the order Docker uses with `--publish`.)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162/files#r27834588
Re: [jclouds-labs] Add template option to specify direct (unmapped)
ports (#162)
Posted by Andrew Donald Kennedy <no...@github.com>.
@andreaturli there are now just two commits, the original and the squashed review changes, which I think preserves enough context without cluttering up the commit history. Or would you prefer squashed to a single commit?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162#issuecomment-90969513
Re: [jclouds-labs] Add template option to specify direct (unmapped)
ports (#162)
Posted by Andrea Turli <no...@github.com>.
Also backported to 1.9.x at http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/38b09ced
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162#issuecomment-91017810
Re: [jclouds-labs] Add template option to specify direct (unmapped)
ports (#162)
Posted by Andrew Donald Kennedy <no...@github.com>.
@andreaturli np, thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162#issuecomment-91013904
Re: [jclouds-labs] Add template option to specify direct (unmapped)
ports (#162)
Posted by Andrew Donald Kennedy <no...@github.com>.
> .toString();
> }
>
> public static final DockerTemplateOptions NONE = new DockerTemplateOptions();
>
> public DockerTemplateOptions volumes(Map<String, String> volumes) {
> - this.volumes = Optional.<Map<String, String>> of(ImmutableMap.copyOf(volumes));
> + for (Map.Entry<String, String> entry : checkNotNull(volumes, "volumes").entrySet()) {
> + checkNotNull(entry.getKey(), "host paths must not be null");
> + checkNotNull(entry.getValue(), "container paths must not be null");
> + }
I couldn't remember. So, do you think we get a useful enogh error when we create an `ImmutableMap` from a `HashMap` with nulls in it? If so, we just need the `checkNotNull(input)` type validation, and I'll update the PR appropriately. _**Checked** and it does a `checkEntryNotNull` so we're all good_
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162/files#r27838342
Re: [jclouds-labs] Add template option to specify direct (unmapped)
ports (#162)
Posted by Andrea Turli <no...@github.com>.
thanks @grkvlt, could you please squash the commits so that I can merge it? thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162#issuecomment-90962193
Re: [jclouds-labs] Add template option to specify direct (unmapped)
ports (#162)
Posted by Andrew Donald Kennedy <no...@github.com>.
@andreaturli is this better?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162#issuecomment-90266830
Re: [jclouds-labs] Add template option to specify direct (unmapped)
ports (#162)
Posted by Andrew Donald Kennedy <no...@github.com>.
@andreaturli @nacx coukld you please give this a quick review?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162#issuecomment-90114323
Re: [jclouds-labs] Add template option to specify direct (unmapped)
ports (#162)
Posted by Andrea Turli <no...@github.com>.
> @@ -136,6 +140,14 @@ public DockerComputeServiceAdapter(DockerApi api) {
> .publishAllPorts(true)
> .privileged(true);
>
> + if (templateOptions.getDirectPorts().isPresent()) {
Maybe it is better to use the docker naming `portBindings` instead of `directPorts`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162/files#r27814405
Re: [jclouds-labs] Add template option to specify direct (unmapped)
ports (#162)
Posted by Andrew Donald Kennedy <no...@github.com>.
Cheers for the quick response @andreaturli @nacx - I have made the changes you suggested.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162#issuecomment-90275744
Re: [jclouds-labs] Add template option to specify direct (unmapped)
ports (#162)
Posted by Andrea Turli <no...@github.com>.
rebuild please
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162#issuecomment-90975459
Re: [jclouds-labs] Add template option to specify direct (unmapped)
ports (#162)
Posted by Andrea Turli <no...@github.com>.
> @@ -136,6 +140,14 @@ public DockerComputeServiceAdapter(DockerApi api) {
> .publishAllPorts(true)
> .privileged(true);
>
> + if (templateOptions.getDirectPorts().isPresent()) {
> + Map<String, List<Map<String, String>>> portBindings = Maps.newHashMap();
> + for (Integer port : templateOptions.getDirectPorts().get()) {
> + portBindings.put(port + "/tcp", Lists.<Map<String, String>>newArrayList(ImmutableMap.of("HostPort", Integer.toString(port))));
Shouldn't check if this `port` has been already specified as `exposedPort` so basically if it is part of `inboundPorts`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162/files#r27814479
Re: [jclouds-labs] Add template option to specify direct (unmapped)
ports (#162)
Posted by Andrew Donald Kennedy <no...@github.com>.
> @@ -136,6 +140,14 @@ public DockerComputeServiceAdapter(DockerApi api) {
> .publishAllPorts(true)
> .privileged(true);
>
> + if (templateOptions.getDirectPorts().isPresent()) {
> + Map<String, List<Map<String, String>>> portBindings = Maps.newHashMap();
> + for (Integer port : templateOptions.getDirectPorts().get()) {
> + portBindings.put(port + "/tcp", Lists.<Map<String, String>>newArrayList(ImmutableMap.of("HostPort", Integer.toString(port))));
It may not have been specified there, though. For example, a Dockerfile will have `EXPOSE` commands, but jclouds does not need to know about them, Docker will find out when it starts the container...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162/files#r27834413
Re: [jclouds-labs] Add template option to specify direct (unmapped)
ports (#162)
Posted by Andrew Donald Kennedy <no...@github.com>.
@andreaturli ok build seems happy now :frog:
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/162#issuecomment-90961150