Skip to content

Conversation

@cdancy
Copy link
Contributor

@cdancy cdancy commented Sep 24, 2016

Resolves #707


This change is Reviewable

String containerName = "generated_" + new SecureRandom().nextInt();

CreateContainerResponse container = dockerClient.createContainerCmd("busybox").withCmd("top")
CreateContainerResponse container = dockerClient.createContainerCmd("busybox").withUser("root").withCmd("top")
Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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();
Copy link
Contributor Author

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 {
Copy link
Contributor Author

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)
Copy link
Member

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?

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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;
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you put @since ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

Copy link
Member

@KostyaSha KostyaSha left a 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 KostyaSha merged commit 0699878 into docker-java:master Sep 30, 2016
@KostyaSha KostyaSha added this to the 3.0.7 milestone Sep 30, 2016
@cdancy
Copy link
Contributor Author

cdancy commented Sep 30, 2016

@KostyaSha thanks!

@KostyaSha
Copy link
Member

Thanks for contribution!

Yogu pushed a commit to Yogu/docker-java that referenced this pull request Nov 15, 2016
panuse pushed a commit to TuKangTech/docker-java that referenced this pull request Aug 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants