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/06/22 20:58:04 UTC
[jclouds-labs] Fixes for Clocker (#178)
Some minor fixes to the Docker provider discovered running Clocker.
- The `DockerTemplateOptions` code needed tidying and incosistencies fixed
- The OkHttp connection should support plaintext as well as TLS, to allow Powerstrip proxying
- The `HostConfig` domain object should support a list of `Dns` strings
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/178
-- Commit Summary --
* Tidy up DockerTemplateOptions and add tests for new options
* Add plaintext option for TLS
* Make Dns a list of strings in HostConfig
-- File Changes --
M docker/src/main/java/org/jclouds/docker/DockerApiMetadata.java (2)
M docker/src/main/java/org/jclouds/docker/compute/options/DockerTemplateOptions.java (88)
M docker/src/main/java/org/jclouds/docker/config/DockerOkHttpClientSupplier.java (6)
M docker/src/main/java/org/jclouds/docker/domain/HostConfig.java (12)
M docker/src/main/java/org/jclouds/docker/suppliers/SSLContextWithKeysSupplier.java (7)
M docker/src/test/java/org/jclouds/docker/compute/options/DockerTemplateOptionsTest.java (46)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/178.patch
https://github.com/jclouds/jclouds-labs/pull/178.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/178
Re: [jclouds-labs] Fixes for Clocker (#178)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @grkvlt! Just two minors, but lgtm. Mind addressing them while squashing the commits to prepare the merge?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/178#issuecomment-116321331
Re: [jclouds-labs] Fixes for Clocker (#178)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -384,7 +400,7 @@ public static DockerTemplateOptions overrideLoginCredentials(LoginCredentials cr
> }
>
> /**
> - * @see TemplateOptions#blockUntilRunning
> + * @see TemplateOptions#blockUntilRunning(boolean)
> */
> public static DockerTemplateOptions blockUntilRunning(boolean blockUntilRunning) {
> DockerTemplateOptions options = new DockerTemplateOptions();
A quick look to all existing template options subclasses show that the only ones that follow this pattern are the ones for Docker, SoftLayer and Azure Compute.
Given that this is not the common pattern and that this PR already cleans up the options in Docker, I think it's better to use this PR to remove the optionals from Docker, and fix SoftLayer and Azure in different PRs.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/178/files#r33062261
Re: [jclouds-labs] Fixes for Clocker (#178)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -384,7 +400,7 @@ public static DockerTemplateOptions overrideLoginCredentials(LoginCredentials cr
> }
>
> /**
> - * @see TemplateOptions#blockUntilRunning
> + * @see TemplateOptions#blockUntilRunning(boolean)
> */
> public static DockerTemplateOptions blockUntilRunning(boolean blockUntilRunning) {
> DockerTemplateOptions options = new DockerTemplateOptions();
What is the purpose of having the options builder methods accepting a `null` value? Is there a need to "clear" any previously set value?
BTW, having a look at this class, it makes no sense to have the variables being an Optional. They're only used internally so they could be perfectly plain nullable objects. That would allow to cleanup the compute service adapter code and make it more readable. WDYT?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/178/files#r32978798
Re: [jclouds-labs] Fixes for Clocker (#178)
Posted by Andrew Donald Kennedy <no...@github.com>.
@andreaturli Does this look OK? What about backporting to the 1.9.x branch?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/178#issuecomment-114221479
Re: [jclouds-labs] Fixes for Clocker (#178)
Posted by Andrea Turli <no...@github.com>.
@grkvlt lgtm
@nacx, is it ok for you as well?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/178#issuecomment-116045970
Re: [jclouds-labs] Fixes for Clocker (#178)
Posted by Andrew Donald Kennedy <no...@github.com>.
> @@ -384,7 +400,7 @@ public static DockerTemplateOptions overrideLoginCredentials(LoginCredentials cr
> }
>
> /**
> - * @see TemplateOptions#blockUntilRunning
> + * @see TemplateOptions#blockUntilRunning(boolean)
> */
> public static DockerTemplateOptions blockUntilRunning(boolean blockUntilRunning) {
> DockerTemplateOptions options = new DockerTemplateOptions();
The code is using the pattern found in other similar classes, see `SoftLayerTemplateOptions` and so on. If we're going to change this would it not make more sense to change _all_ of these in a separate pull request?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/178/files#r32994528
Re: [jclouds-labs] Fixes for Clocker (#178)
Posted by Zack Shoylev <no...@github.com>.
> * ComputeService api = // get connection
> * templateBuilder.options(inboundPorts(22, 80, 8080, 443));
> - * Set<? extends NodeMetadata> set = api.createNodesInGroup(tag, 2, templateBuilder.build());
> - * <code>
> + * Set<? extends NodeMetadata> set = api.createNodesInGroup(tag, 2, templateBuilder.build());}</pre>
> */
> public class DockerTemplateOptions extends TemplateOptions implements Cloneable {
This is reasonable enough. Thanks @nacx
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/178/files#r32982976
Re: [jclouds-labs] Fixes for Clocker (#178)
Posted by Ignasi Barrera <no...@github.com>.
> * ComputeService api = // get connection
> * templateBuilder.options(inboundPorts(22, 80, 8080, 443));
> - * Set<? extends NodeMetadata> set = api.createNodesInGroup(tag, 2, templateBuilder.build());
> - * <code>
> + * Set<? extends NodeMetadata> set = api.createNodesInGroup(tag, 2, templateBuilder.build());}</pre>
> */
> public class DockerTemplateOptions extends TemplateOptions implements Cloneable {
Given that this extends the `TemplateOptions` class I'd say to leave this as-is, to avoid having a mixture of styles in one single class.
I agree this is something we can (and should) to in future PRs, but changing the TemplateOptions class in compute and all providers, but that is out of the scope of this PR.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/178/files#r32982211
Re: [jclouds-labs] Fixes for Clocker (#178)
Posted by Andrea Turli <no...@github.com>.
Lgtm
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/178#issuecomment-114223539
Re: [jclouds-labs] Fixes for Clocker (#178)
Posted by Zack Shoylev <no...@github.com>.
> * ComputeService api = // get connection
> * templateBuilder.options(inboundPorts(22, 80, 8080, 443));
> - * Set<? extends NodeMetadata> set = api.createNodesInGroup(tag, 2, templateBuilder.build());
> - * <code>
> + * Set<? extends NodeMetadata> set = api.createNodesInGroup(tag, 2, templateBuilder.build());}</pre>
> */
> public class DockerTemplateOptions extends TemplateOptions implements Cloneable {
It might be better to refactor this to autovalue and autovalue builders?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/178/files#r32981797
Re: [jclouds-labs] Fixes for Clocker (#178)
Posted by Andrew Donald Kennedy <no...@github.com>.
@nacx Thanks for the review, have squashed the commits and made the changes you suggested.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/178#issuecomment-116329075
Re: [jclouds-labs] Fixes for Clocker (#178)
Posted by Andrew Donald Kennedy <no...@github.com>.
> @@ -384,7 +400,7 @@ public static DockerTemplateOptions overrideLoginCredentials(LoginCredentials cr
> }
>
> /**
> - * @see TemplateOptions#blockUntilRunning
> + * @see TemplateOptions#blockUntilRunning(boolean)
> */
> public static DockerTemplateOptions blockUntilRunning(boolean blockUntilRunning) {
> DockerTemplateOptions options = new DockerTemplateOptions();
OK, thanks @nacx I'll fix that here, and SoftLayer and Azure can be done elsewhere. I hadn't realised my choice of template option classes was so unusual!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/178/files#r33062969
Re: [jclouds-labs] Fixes for Clocker (#178)
Posted by Ignasi Barrera <no...@github.com>.
Pushed to [master](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/7c5ded68) and [1.9.x](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/dcd1dc15). Thanks @grkvlt!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/178#issuecomment-116573807
Re: [jclouds-labs] Fixes for Clocker (#178)
Posted by Ignasi Barrera <no...@github.com>.
> }
>
> - if (templateOptions.getMemory().isPresent()) {
> - containerConfigBuilder.memory(templateOptions.getMemory().get());
> + if (templateOptions.getMemory() != null) {
Simplify this by removing the null check conditionals? These methods accept null values, so let's call them directly.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/178/files#r33426261
Re: [jclouds-labs] Fixes for Clocker (#178)
Posted by Ignasi Barrera <no...@github.com>.
Closed #178.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/178#event-342796062
Re: [jclouds-labs] Fixes for Clocker (#178)
Posted by Ignasi Barrera <no...@github.com>.
> }
> - if (hostname.isPresent()) {
> - eTo.hostname(hostname.get());
> + if (hostname != null) {
The null conditionals can be removed as these properties are `nullable` and there's no null check.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/178/files#r33426250
Re: [jclouds-labs] Fixes for Clocker (#178)
Posted by Ignasi Barrera <no...@github.com>.
Mind remove the conditionals [here](https://github.com/grkvlt/jclouds-labs/blob/fix/clocker/docker/src/main/java/org/jclouds/docker/compute/strategy/DockerComputeServiceAdapter.java#L104-L114) too?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/178#issuecomment-116332127
Re: [jclouds-labs] Fixes for Clocker (#178)
Posted by Andrew Donald Kennedy <no...@github.com>.
@nacx Good catch; done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/178#issuecomment-116332937
Re: [jclouds-labs] Fixes for Clocker (#178)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -384,7 +400,7 @@ public static DockerTemplateOptions overrideLoginCredentials(LoginCredentials cr
> }
>
> /**
> - * @see TemplateOptions#blockUntilRunning
> + * @see TemplateOptions#blockUntilRunning(boolean)
> */
> public static DockerTemplateOptions blockUntilRunning(boolean blockUntilRunning) {
> DockerTemplateOptions options = new DockerTemplateOptions();
I wouldn't say unusual but "recent providers". I didn't notice this pattern was being used until I've had a deeper look at this PR. I'll take care of fixing SoftLayer and Azure. Thanks for spotting!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/178/files#r33063442
Re: [jclouds-labs] Fixes for Clocker (#178)
Posted by Andrew Donald Kennedy <no...@github.com>.
@nacx @andreaturli I have changed the classes to remove the use of `Optional` and also updated the tests to reflect the change to `HostConfig`, is there anything else I need to do to make this available for merging with 1.9.1-SNAPSHOT as well as 2.0.0-SNAPSHOT?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/178#issuecomment-115924263