You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Duncan Grant <no...@github.com> on 2016/06/09 08:39:45 UTC
[jclouds/jclouds-labs] Node json should be optional (#281)
The Node element of the container json is optional as well as nullable.
In PR #277 I tested against docker-engine provisioned through docker-machine and docker-swarm but not against vanilla docker engine. I have now tested against all three.
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/281
-- Commit Summary --
* Node json should be optional
-- File Changes --
M docker/src/main/java/org/jclouds/docker/compute/functions/ContainerToNodeMetadata.java (5)
M docker/src/main/java/org/jclouds/docker/domain/Container.java (9)
M docker/src/test/java/org/jclouds/docker/compute/functions/ContainerToNodeMetadataTest.java (3)
M docker/src/test/java/org/jclouds/docker/parse/ContainerParseTest.java (5)
M docker/src/test/java/org/jclouds/docker/parse/ContainerVersionMajor1Minor21.java (3)
M docker/src/test/resources/container.json (3)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/281.patch
https://github.com/jclouds/jclouds-labs/pull/281.diff
---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/281
Re: [jclouds/jclouds-labs] Node json should be optional (#281)
Posted by Andrew Donald Kennedy <no...@github.com>.
> @@ -266,7 +267,7 @@ public Builder fromContainer(Container in) {
> .sysInitPath(in.sysInitPath()).resolvConfPath(in.resolvConfPath()).driver(in.driver())
> .execDriver(in.execDriver()).volumes(in.volumes()).hostConfig(in.hostConfig()).volumesRW(in.volumesRW())
> .command(in.command()).status(in.status()).ports(in.ports()).hostnamePath(in.hostnamePath())
> - .hostsPath(in.hostsPath()).mountLabel(in.mountLabel()).processLabel(in.processLabel()).node(in.node());
> + .hostsPath(in.hostsPath()).mountLabel(in.mountLabel()).processLabel(in.processLabel()).node(in.node().get());
This will fail when `node` is `Optional.absent()`
---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/281/files/74af80710c0a4456dc1cabea949123ead7a54009#r66759899
Re: [jclouds/jclouds-labs] Node json should be optional (#281)
Posted by Ignasi Barrera <no...@github.com>.
Pushed to master as [f47223b5](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/f47223b5). Thanks!
---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/281#issuecomment-225551472
Re: [jclouds/jclouds-labs] Node json should be optional (#281)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -113,8 +112,8 @@ private String cleanUpName(String name) {
>
> private List<String> getPublicIpAddresses(Container container) {
> String dockerIpAddress;
> - if (container.node() != null && !Strings.isNullOrEmpty(container.node().ip())) {
> - dockerIpAddress = container.node().ip();
> + if (container.node() != null && container.node().isPresent()) {
Remove the `!= null` check as it will never be `null`.
---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/281/files/60f96629c0f24dc819a526c5347f70beb0e7f416#r66768217
Re: [jclouds/jclouds-labs] Node json should be optional (#281)
Posted by Andrew Donald Kennedy <no...@github.com>.
@duncangrant Thanks, good catch! The `Optional<?>` type in autovalue is a nice feature!
---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/281#issuecomment-224873637
Re: [jclouds/jclouds-labs] Node json should be optional (#281)
Posted by Ignasi Barrera <no...@github.com>.
Closed #281.
---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/281#event-690277162
Re: [jclouds/jclouds-labs] Node json should be optional (#281)
Posted by Andrew Donald Kennedy <no...@github.com>.
@andreaturli This is _another_ Swarm related patch (for master and 1.9.x) that fixes an issue we missed previously, and @duncangrant has now added some tests, too. Can you take a look?
---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/281#issuecomment-224873808
Re: [jclouds/jclouds-labs] Node json should be optional (#281)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -113,8 +112,8 @@ private String cleanUpName(String name) {
>
> private List<String> getPublicIpAddresses(Container container) {
> String dockerIpAddress;
> - if (container.node() != null && !Strings.isNullOrEmpty(container.node().ip())) {
> - dockerIpAddress = container.node().ip();
> + if (container.node() != null && container.node().isPresent()) {
If using optionals we should get rid of the null check and assume it is never null.
---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/281/files/60f96629c0f24dc819a526c5347f70beb0e7f416#r66576066
Re: [jclouds/jclouds-labs] Node json should be optional (#281)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -77,7 +78,7 @@
>
> @Nullable public abstract String processLabel();
>
> - @Nullable public abstract Node node();
> + @Nullable public abstract Optional<Node> node();
Optional fields should never be nullable.
---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/281/files/60f96629c0f24dc819a526c5347f70beb0e7f416#r66575804
Re: [jclouds/jclouds-labs] Node json should be optional (#281)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -249,7 +250,7 @@ public Builder processLabel(String processLabel) {
> return this;
> }
>
> - public Builder node(Node node) {
> + public Builder node(Optional<Node> node) {
I'd keep the build interface receiving a node and assign it with `Optional.fromNullable(node)`.
---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/281/files/60f96629c0f24dc819a526c5347f70beb0e7f416#r66575967
Re: [jclouds/jclouds-labs] Node json should be optional (#281)
Posted by Andrew Donald Kennedy <no...@github.com>.
@duncangrant LGTM @nacx can you take another look?
---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/281#issuecomment-225538900
Re: [jclouds/jclouds-labs] Node json should be optional (#281)
Posted by Andrew Donald Kennedy <no...@github.com>.
@duncangrant can you also make a PR for the 1.9.x branch, please?
---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/281#issuecomment-225564410
Re: [jclouds/jclouds-labs] Node json should be optional (#281)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -93,7 +94,7 @@ public static Container create(String id, Date created, String path, String name
> String resolvConfPath, Map<String, String> volumes, HostConfig hostConfig,
> String driver, String execDriver, Map<String, Boolean> volumesRW, String command,
> String status, List<Port> ports, String hostnamePath, String hostsPath,
> - String mountLabel, String processLabel, Node node) {
> + String mountLabel, String processLabel, Optional<Node> node) {
> return new AutoValue_Container(id, created, path, name, copyOf(args), config, state, image, networkSettings,
> sysInitPath, resolvConfPath, copyOf(volumes), hostConfig, driver, execDriver, copyOf(volumesRW), command,
> status, copyOf(ports), hostnamePath, hostsPath, mountLabel, processLabel, node);
Use `Optional.fromNullable(node)` to set the node.
---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/281/files/60f96629c0f24dc819a526c5347f70beb0e7f416#r66575860
Re: [jclouds/jclouds-labs] Node json should be optional (#281)
Posted by Ignasi Barrera <no...@github.com>.
Just one last minor comment. Let's address this and squash directly so we can cleanly merge the PR. Thanks @duncangrant @grkvlt!
---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/281#issuecomment-225541309
Re: [jclouds/jclouds-labs] Node json should be optional (#281)
Posted by Duncan Grant <no...@github.com>.
@grkvlt @nacx Thanks for you comments. I think I've addressed them.
---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/281#issuecomment-225159523
Re: [jclouds/jclouds-labs] Node json should be optional (#281)
Posted by Duncan Grant <no...@github.com>.
Addressed and merged.
thanks Duncan
---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/281#issuecomment-225547902