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 2014/10/10 19:57:34 UTC
[jclouds-labs] Minor Docker Fixes (#89)
Fix some minor issues in the Docker driver
You can merge this Pull Request by running:
git pull https://github.com/grkvlt/jclouds-labs grkvlt/docker
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs/pull/89
-- Commit Summary --
* Entrypoint should be a JSON array
* Fix Container conversion when image ID not present
* Set hostname in template options
-- File Changes --
M docker/src/main/java/org/jclouds/docker/compute/functions/ContainerToNodeMetadata.java (10)
M docker/src/main/java/org/jclouds/docker/compute/strategy/DockerComputeServiceAdapter.java (4)
M docker/src/main/java/org/jclouds/docker/domain/Config.java (13)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/89.patch
https://github.com/jclouds/jclouds-labs/pull/89.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by Adrian Cole <no...@github.com>.
> @@ -121,6 +125,6 @@ protected static int getLoginPort(Container container) {
> }
> }
> }
> - throw new IllegalStateException("Cannot determine the login port for " + container.getId());
> + return -1;
yeah i figured, hence the pre-emptive +1
doc sounds fine. thx for looking (and the fixes!)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89/files#r18727737
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by Andrew Phillips <no...@github.com>.
> public static class Builder {
>
> - /**
> - * @see DockerTemplateOptions#volumes(java.util.Map)
> - */
> - public static DockerTemplateOptions volumes(Map<String, String> volumes) {
> - DockerTemplateOptions options = new DockerTemplateOptions();
> - return DockerTemplateOptions.class.cast(options.volumes(volumes));
> - }
> + /**
> + * @see DockerTemplateOptions#volumes(java.util.Map)
> + */
> + public static DockerTemplateOptions volumes(Map<String, String> volumes) {
> + DockerTemplateOptions options = new DockerTemplateOptions();
> + return DockerTemplateOptions.class.cast(options.volumes(volumes));
> + }
[minor] Intentional change?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89/files#r18745177
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by Andrew Donald Kennedy <no...@github.com>.
@nacx The image cache change sounds useful, and would definitely help in the Docker situation with frequent commits happening. Look forward to it...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-58769884
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1670](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1670/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-58697448
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by Andrew Donald Kennedy <no...@github.com>.
> @@ -121,6 +125,6 @@ protected static int getLoginPort(Container container) {
> }
> }
> }
> - throw new IllegalStateException("Cannot determine the login port for " + container.getId());
> + return -1;
Good point, will fix.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89/files#r18727177
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by Adrian Cole <no...@github.com>.
> @@ -121,6 +125,6 @@ protected static int getLoginPort(Container container) {
> }
> }
> }
> - throw new IllegalStateException("Cannot determine the login port for " + container.getId());
> + return -1;
Hmm Optional is probably better than -1 these days. Or NoSuchElementException
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89/files#r18723300
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by Andrew Donald Kennedy <no...@github.com>.
@nacx @demobox @andreaturli Suggest ready to merge?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-59496509
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by Ignasi Barrera <no...@github.com>.
Regarding the image ID thing, it is caused by the current image cache. I started working on [JCLOUDS-512](https://issues.apache.org/jira/browse/JCLOUDS-512) to fix that, but it is a bit complex as there are providers, such as AWS, that cache images differently. I can't give an ETA, but I hope we'll have that fixed soon.
With the actual (partial) fix, you should be able to get the new image using the TemplateBuilder and providing the imageId. But the listImages and ImageExtension fixes are still a WIP. Meanwhile there is the (not good) workaround to close and reopen the context again.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-58740802
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1679](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1679/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-58770195
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by Andrew Donald Kennedy <no...@github.com>.
> public static class Builder {
>
> - /**
> - * @see DockerTemplateOptions#volumes(java.util.Map)
> - */
> - public static DockerTemplateOptions volumes(Map<String, String> volumes) {
> - DockerTemplateOptions options = new DockerTemplateOptions();
> - return DockerTemplateOptions.class.cast(options.volumes(volumes));
> - }
> + /**
> + * @see DockerTemplateOptions#volumes(java.util.Map)
> + */
> + public static DockerTemplateOptions volumes(Map<String, String> volumes) {
> + DockerTemplateOptions options = new DockerTemplateOptions();
> + return DockerTemplateOptions.class.cast(options.volumes(volumes));
> + }
I think I was fixing the indents to match rest of file
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89/files#r19011379
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1674](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1674/) UNSTABLE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-58755122
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by Andrew Phillips <no...@github.com>.
Thanks for the explanation, @grkvlt! Glad to see we're able to help weave out...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-58751426
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #307](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/307/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-58711613
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by Andrew Donald Kennedy <no...@github.com>.
@demobox The test change required was to modify a JSON file to use an array of Strings for Entrypoint instead of null. This would have failed the test by crashing the JSON parser, but it now passes. I think documenting the DTOs is not strictly necessary, they're not created by users, rather filled in from returned API data. If we decide we need it, I'd rather document *everything* in another PR
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-59496480
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by Ignasi Barrera <no...@github.com>.
>@grkvlt Do you think it makes sense to wait for the image cache @nacx talks about?
I think there is no need to block this PR until the image cache is refactored. The refactor will mitigate the side-effects that appear now when listing images, but I don't see these side-effects as blockers for other changes to be merged. I'd say to go ahead and merge this PR once the tests are completed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-58824314
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by Andrew Donald Kennedy <no...@github.com>.
Hi @demobox
These are just things I discovered while writing an integration with github.com/zettio/weave which auto-creates containers that have no SSH daemon, hence no inbound port 22, and jclouds still wants to enumerate them.
The image ID issue happens because jclouds is cacheing image data, and when you are committing changes in stages to a container a new image with a new ID is created, which cannot then be found. The original image ID you started from is still there, but intermediate ones are not. Maybe this should have been solved by changing the cache parameters for images in the Docker provider?
I will add some updated tests before this is merged...
I believe @andreaturli is working on updates to handle the new 1.14 API in the next release of Docker.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-58725712
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #313](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/313/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-58769976
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #308](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/308/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-58754717
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #305](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/305/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-58693688
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1675](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1675/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-58757695
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by Adrian Cole <no...@github.com>.
LGTM
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-58700075
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by Andrew Donald Kennedy <no...@github.com>.
I updated one of the test JSON files with some `Entrypoint` data to test the parser for `Config` objects, so this ought to be mergeable now.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-58769862
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1671](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1671/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-58713020
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by Andrew Donald Kennedy <no...@github.com>.
@andreaturli Thanks
@nacx Let me know when the image changes are ready, and I'll update the provider to use the cache properly
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-59559351
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1669](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1669/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-58695244
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by Andrew Phillips <no...@github.com>.
> @@ -194,7 +194,7 @@ public String getWorkingDir() {
> return workingDir;
> }
>
> - public String getEntrypoint() {
> + public List<String> getEntrypoint() {
Do we have/need some documentation to describe what is expected here? Reading "entrypoint" but seeing a _list_ being required, it may not be immediately clear what kind of input is expected.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89/files#r18745184
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by Andrew Phillips <no...@github.com>.
@grkvlt Do you think it makes sense to wait for the image cache @nacx talks about? If not, do we still need some tests here before merging?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-58770177
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by Andrew Donald Kennedy <no...@github.com>.
> @@ -121,6 +125,6 @@ protected static int getLoginPort(Container container) {
> }
> }
> }
> - throw new IllegalStateException("Cannot determine the login port for " + container.getId());
> + return -1;
Nope, it looks like the ripple effect of changes is too big, particularly since its only Docker that can ever have an image (container) that you cannot log into. I think it's safer to leave the magic value here, but perhaps document the Docker class explaining what it means?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89/files#r18727590
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by Andrew Phillips <no...@github.com>.
This is labs so I'm +1 with merging this straight away, but just wanted to ask: do we need to add/update any tests for this? Also, could you just explain the background a bit, @grkvlt? Are these bugfixes or changes to accommodate new features in Docker or..?
Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-58719360
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #309](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/309/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-58757278
Re: [jclouds-labs] Minor Docker Fixes (#89)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #306](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/306/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/89#issuecomment-58694955