-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Expose 'User' property on ExecCreateCmd (#707) #708
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| String containerName = "generated_" + new SecureRandom().nextInt(); | ||
|
|
||
| CreateContainerResponse container = dockerClient.createContainerCmd("busybox").withCmd("top") | ||
| CreateContainerResponse container = dockerClient.createContainerCmd("busybox").withUser("root").withCmd("top") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add user option here to show that creating the command works as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make separate test, root will be used by default afair.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found, depends on when param was added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. I guess my thinking was that setting the user, even though it's the default, would show that it works as expected without having to create a new test for this one option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, let's keep
|
|
||
| ExecCreateCmdResponse execCreateCmdResponse = dockerClient.execCreateCmd(container.getId()) | ||
| .withAttachStdout(true).withCmd("touch", "/execStartTest.log").exec(); | ||
| .withAttachStdout(true).withCmd("touch", "/execStartTest.log").withUser("root").exec(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added User here to show that not only creating the command works but that executing with the user does as well.
| } | ||
|
|
||
| @Test(groups = "ignoreInCircleCi", expectedExceptions = NotFoundException.class) | ||
| public void execStartWithNonExistentUser() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test to show that using a non-existent name fails.
| assertTrue(responseAsString.length() > 0); | ||
| } | ||
|
|
||
| @Test(groups = "ignoreInCircleCi", expectedExceptions = NotFoundException.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where expectedExceptions = NotFoundException.class is expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not clear because last line in test is assert so exception didn't happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things are not exactly clear here I admit and now I'm scratching my head wondering why I did things this way. Let me clean this up a bit along with the other minor changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so it's the below line that throws the NFE. My thinking was to remove everything below this line and keep the expectedExceptions = NotFoundException.class. This shows that creating with some non-existent-user disallows one to copy anything from the container, in this case /execStartTest.log, because it does not exist because the user itself does not exist.
InputStream response = dockerClient.copyArchiveFromContainerCmd(container.getId(), "/execStartTest.log").exec();
| private Boolean tty; | ||
|
|
||
| @JsonProperty("User") | ||
| private String user; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since what api version it appeared? existed all the time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merged for docker 1.7.0 which is analogous to api version 1.19.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you put @since ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
KostyaSha
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Api version, exception place.
|
@KostyaSha thanks! |
|
Thanks for contribution! |
…a#708) * Expose 'User' property on ExecCreateCmd (docker-java#707)
…a#708) * Expose 'User' property on ExecCreateCmd (docker-java#707)
Resolves #707
This change is