Skip to content

DOCKER_TLS_VERIFY can be 'false' or empty. #520

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.

Sign up for GitHub

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

Closed
hongweiyi wants to merge 4 commits into from
Closed

DOCKER_TLS_VERIFY can be 'false' or empty. #520

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
91f8423
Fix Issue #519
hongweiyi Mar 23, 2016
a0d8726
Improve #519 test case.
hongweiyi Mar 23, 2016
3a1a66b
Improve imports
hongweiyi Mar 23, 2016
c7ad7df
Improve imports again.
hongweiyi Mar 23, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/main/java/com/github/dockerjava/core/DockerClientConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import java.util.Properties;
import java.util.Set;

import org.apache.commons.lang.BooleanUtils;
import org.apache.commons.lang.StringUtils;
import org.apache.commons.lang.builder.EqualsBuilder;
import org.apache.commons.lang.builder.HashCodeBuilder;
Expand Down Expand Up @@ -418,8 +417,8 @@ public final DockerClientConfigBuilder withDockerConfig(String dockerConfig) {
}

public final DockerClientConfigBuilder withDockerTlsVerify(String dockerTlsVerify) {
this.dockerTlsVerify = BooleanUtils.toBoolean(dockerTlsVerify.trim())
|| BooleanUtils.toBoolean(dockerTlsVerify.trim(), "1", "0");
String trimmed = dockerTlsVerify.trim();
this.dockerTlsVerify = "true".equals(trimmed) || "1".equals(trimmed);
Copy link
Copy Markdown
Member

@KostyaSha KostyaSha Mar 23, 2016

Choose a reason for hiding this comment

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

Looks not very good for eyes, have you looked at org.apache.commons.lang.BooleanUtils#toBooleanObject(java.lang.String) ?

Copy link
Copy Markdown
Contributor Author

@hongweiyi hongweiyi Mar 23, 2016

Choose a reason for hiding this comment

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

Maybe org.apache.commons.lang.BooleanUtils#toBooleanObject(java.lang.String) is too complicated in this case, "true".equals(trimmed) || "1".equals(trimmed) is simple, fast and straight forward.

Copy link
Copy Markdown
Member

@KostyaSha KostyaSha Mar 23, 2016

Choose a reason for hiding this comment

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

Well, all Utils are available to simplify the code readability and i think one of them should work. Can't get idea what is wrong 🏧 and why utils can't be used... toBoolean() calls toBooleanObject()

Copy link
Copy Markdown
Member

@KostyaSha KostyaSha Mar 23, 2016

Choose a reason for hiding this comment

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

and imho TRUE should also work as true, so better to use existed utils, they are already verified and covers different corner cases.

Copy link
Copy Markdown
Contributor Author

@hongweiyi hongweiyi Mar 23, 2016

Choose a reason for hiding this comment

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

LOL, there always two ways. 1. make maximum use of Utils; 2. make code as simple as possible.

But I'm the latter one, I perfect has third libraries as little as possible.

As the former one, if BooleanUtils has a method to pass default value, it will looks very good for eyes, just like:

toBoolean(str, trueStr, falseStr, defaultValue);

Or if docker-java does not want user to set invalid value to DOCKER_TLS_VERIFY, it should throw exception directly.

return this;
}

Expand Down
22 changes: 22 additions & 0 deletions src/test/java/com/github/dockerjava/core/DockerClientConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertFalse;
import static org.testng.Assert.assertTrue;

import java.lang.reflect.Field;
import java.net.URI;
import java.util.Collections;
import java.util.HashMap;
Expand Down Expand Up @@ -176,4 +179,23 @@ public void testUnixHostScheme() throws Exception {
new DockerClientConfig(URI.create("unix://foo"), "dockerConfig", "apiVersion", "registryUrl", "registryUsername", "registryPassword", "registryEmail",
null, false);
}

@Test
public void withDockerTlsVerify() throws Exception {
DockerClientConfig.DockerClientConfigBuilder builder = new DockerClientConfig.DockerClientConfigBuilder();
Field field = builder.getClass().getDeclaredField("dockerTlsVerify");
field.setAccessible(true);

builder.withDockerTlsVerify("");
assertFalse(field.getBoolean(builder));
builder.withDockerTlsVerify("false");
assertFalse(field.getBoolean(builder));
builder.withDockerTlsVerify("true");
assertTrue(field.getBoolean(builder));
builder.withDockerTlsVerify("0");
assertFalse(field.getBoolean(builder));
builder.withDockerTlsVerify("1");
assertTrue(field.getBoolean(builder));
}

}