You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Josef Cacek <no...@github.com> on 2015/09/28 23:36:37 UTC
[jclouds-labs] [JCLOUDS-1007] Implemented Docker Exec support in
MiscApi (#205)
JIRA: https://issues.apache.org/jira/browse/JCLOUDS-1007
Two new methods were added to `MiscApi`:
* `execCreate` - Sets up an exec instance in a running container with given Id;
* `execStart` - Starts a previously set up exec instance id.
Additional support for Docker multiplexed standard stream (IN, OUT, ERR) was added in form of `DockerInputStream`.
Usage:
```java
ExecCreateParams execCreateParams = ExecCreateParams.builder()
.cmd(ImmutableList.<String> of("/bin/sh", "-c", "echo -n Standard >&1 && echo -n Error >&2"))
.attachStderr(true).attachStdout(true).build()
Exec exec = miscApi.execCreate(container.id(), execCreateParams);
ExecStartParams execStartParams = ExecStartParams.builder().detach(false).build();
try (DockerInputStream inputStream = new DockerInputStream(api().execStart(exec.id(), startParams))) {
StdStreamData data = null;
while (null != (data = inputStream.readStdStreamData())) {
byte[] payload = data.getPayload();
switch (data.getType()) {
case OUT:
// process stadard output
break;
case ERR:
// process error output
break;
default:
throw new IllegalArgumentException();
break;
}
}
}
```
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/205
-- Commit Summary --
* [JCLOUDS-1007] Implemented Docker Exec support in MiscApi
-- File Changes --
A docker/src/main/java/org/jclouds/docker/domain/Exec.java (35)
A docker/src/main/java/org/jclouds/docker/domain/ExecCreateParams.java (65)
A docker/src/main/java/org/jclouds/docker/domain/ExecStartParams.java (47)
M docker/src/main/java/org/jclouds/docker/features/MiscApi.java (38)
A docker/src/main/java/org/jclouds/docker/util/DockerInputStream.java (73)
A docker/src/main/java/org/jclouds/docker/util/StdStreamData.java (87)
M docker/src/test/java/org/jclouds/docker/features/MiscApiLiveTest.java (96)
M docker/src/test/java/org/jclouds/docker/features/MiscApiMockTest.java (54)
A docker/src/test/resources/exec.json (1)
A docker/src/test/resources/exec.start (0)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/205.patch
https://github.com/jclouds/jclouds-labs/pull/205.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/205
Re: [jclouds-labs] [JCLOUDS-1007] Implemented Docker Exec support in
MiscApi (#205)
Posted by Ignasi Barrera <no...@github.com>.
> + @SerializedNames({ "Detach" })
> + public static ExecStartParams create(boolean detach) {
> + return builder().detach(detach).build();
> + }
> +
> + public static Builder builder() {
> + return new AutoValue_ExecStartParams.Builder().detach(false);
> + }
> +
> + @AutoValue.Builder
> + public abstract static class Builder {
> +
> + public abstract Builder detach(boolean b);
> +
> + public abstract ExecStartParams build();
> + }
Some domain classes have builders, some not. Should we remove all the builders, given that these classes are pretty simple, for consistency?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/205/files#r40972202
Re: [jclouds-labs] [JCLOUDS-1007] Implemented Docker Exec support in
MiscApi (#205)
Posted by Andrew Donald Kennedy <no...@github.com>.
@kwart Looks good, this will be useful for Clocker too, thanks.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/205#issuecomment-145030966
Re: [jclouds-labs] [JCLOUDS-1007] Implemented Docker Exec support in
MiscApi (#205)
Posted by Josef Cacek <no...@github.com>.
@nacx, thank you for the review.
I'll rewrite the builder with the immutable `List` implemented according to sample you've linked.
I would keep the builders in the new domain classes. IMO they improve code readability. Even it's more talkative, it's clearer to use
```java
ExecStartParams.builder().detach(false).build();
```
than
```java
ExecStartParams.create(false);
```
It's also simpler to keep API backward compatible with the builders. E.g. if somebody will add [tty attribute](https://docs.docker.com/reference/api/docker_remote_api_v1.16/#exec-create) implementation to `ExecCreateParams` class, then instead of introducing new argument of the `create()` method, one new method to Builder's flow API will be added.
The reason why the builders were not included in all domain classes was the fact that some of them are not intended to be used by users directly. They are used only as a return values from remote API calls.
I'll add the fixes as a new commit and if it's OK, I can squash it into one after the review.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/205#issuecomment-144938981
Re: [jclouds-labs] [JCLOUDS-1007] Implemented Docker Exec support in
MiscApi (#205)
Posted by Ignasi Barrera <no...@github.com>.
> + * {@link #attachStderr()} and {@link #attachStdout()} to true as a default.
> + *
> + * @return new {@link ExecCreateParams.Builder} instance
> + */
> + public static Builder builder() {
> + return new AutoValue_ExecCreateParams.Builder().attachStderr(true).attachStdout(true);
> + }
> +
> + @AutoValue.Builder
> + public abstract static class Builder {
> +
> + public abstract Builder attachStdout(boolean b);
> +
> + public abstract Builder attachStderr(boolean b);
> +
> + public abstract Builder cmd(List<String> cmds);
We want lists to be immutable, but we don't want to force an implementation in the signature. Consider using [this builder pattern](https://github.com/jclouds/jclouds-labs-openstack/blob/master/openstack-heat/src/main/java/org/jclouds/openstack/heat/v1/options/CreateStack.java#L95-L115) to make the builder produce immutable lists. The trick is to add getters that are only visible to the builder itself but provide access to the actual list, so we can generate a proper immutable one when building, regardless of the list the used passed when calling the public builder methods.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/205/files#r40972128
Re: [jclouds-labs] [JCLOUDS-1007] Implemented Docker Exec support in
MiscApi (#205)
Posted by Ignasi Barrera <no...@github.com>.
This is really nice @kwart. There just a couple minor comments about the builders, but apart from that lgtm. Loving the stream test :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/205#issuecomment-144864704
Re: [jclouds-labs] [JCLOUDS-1007] Implemented Docker Exec support in
MiscApi (#205)
Posted by Josef Cacek <no...@github.com>.
Rebased to latest master.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/205#issuecomment-145037271
Re: [jclouds-labs] [JCLOUDS-1007] Implemented Docker Exec support in
MiscApi (#205)
Posted by Josef Cacek <no...@github.com>.
I've updated the PR and added also method `execInspect` - it's needed to retrieve exit code from the exec instance.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/205#issuecomment-144049731
Re: [jclouds-labs] [JCLOUDS-1007] Implemented Docker Exec support in
MiscApi (#205)
Posted by Ignasi Barrera <no...@github.com>.
Pushed to master as [e2a4ca0d](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/e2a4ca0d). Thanks @kwart!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/205#issuecomment-145041416
Re: [jclouds-labs] [JCLOUDS-1007] Implemented Docker Exec support in
MiscApi (#205)
Posted by Ignasi Barrera <no...@github.com>.
Closed #205.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/205#event-425316568
Re: [jclouds-labs] [JCLOUDS-1007] Implemented Docker Exec support in
MiscApi (#205)
Posted by Ignasi Barrera <no...@github.com>.
Thanks for the explanation. Changes will be minors and the PR is not big, so feel free to squash the commits directly :)
Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/205#issuecomment-144940042