Throw IllegalArgumentException when jib.to.tags is set to a collection containing a null value. #2761
Conversation
…ned to Set<String> containing null value
| ConfigurationPropertyValidator.parseListProperty( | ||
| System.getProperty(PropertyNames.TO_TAGS))); | ||
| String property = System.getProperty(PropertyNames.TO_TAGS); | ||
| Set<String> tags = |
There was a problem hiding this comment.
For readability: I think maybe tags is confusing with the class member tags (this.tags). Perhaps use something that differentiates it? Personally I don't think this.tags should be used very often outside constructors.
| ConfigurationPropertyValidator.parseListProperty( | ||
| System.getProperty(PropertyNames.TO_TAGS))) | ||
| : this.tags; | ||
| if (tags.stream().anyMatch(Strings::isNullOrEmpty)) { |
There was a problem hiding this comment.
should we handle whitespace?
There was a problem hiding this comment.
Oh that didn't occur to me. Thanks!
|
|
||
| // Testing scenario where jib.to.tags is set to a collection containing a null value. | ||
| HashSet<String> tags = new HashSet<String>(); | ||
| tags.add(null); |
There was a problem hiding this comment.
if we fail on empty, we should probably add a test for empty as well
There was a problem hiding this comment.
Also the case of getting tags from the system property.
| Assert.assertEquals("some username", testJibExtension.getTo().getAuth().getUsername()); | ||
| Assert.assertEquals("some password", testJibExtension.getTo().getAuth().getPassword()); | ||
|
|
||
| // Testing scenario where jib.to.tags is set to a collection containing a null value. |
There was a problem hiding this comment.
Maybe just put this in a new test?
|
|
||
| // Testing scenario where jib.to.tags is set to a collection containing a null value. | ||
| HashSet<String> tags = new HashSet<String>(); | ||
| tags.add(null); |
There was a problem hiding this comment.
Also the case of getting tags from the system property.
| to.setTags(tags); | ||
| }); | ||
| testJibExtension.getTo().getTags(); | ||
| Assert.fail("Expect this to fail"); |
There was a problem hiding this comment.
Nit: if the message doesn't provide additional helpful information or details, just use Assert.fail().
And I think it's meaningful to move testJibExtension.to() out of the try-catch block.
There was a problem hiding this comment.
Ah yes, having all the extra lines outside the try-catch makes sense. Thanks!
| property != null | ||
| ? ImmutableSet.copyOf( | ||
| ConfigurationPropertyValidator.parseListProperty( | ||
| System.getProperty(PropertyNames.TO_TAGS))) |
There was a problem hiding this comment.
| System.getProperty(PropertyNames.TO_TAGS))) | |
| property)) |
jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibExtensionTest.java
Outdated
Show resolved
Hide resolved
jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/TargetImageParameters.java
Outdated
Show resolved
Hide resolved
…dle/JibExtensionTest.java Co-authored-by: Chanseok Oh <chanseok@google.com>
…dle/TargetImageParameters.java Co-authored-by: Chanseok Oh <chanseok@google.com>
jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/TargetImageParameters.java
Outdated
Show resolved
Hide resolved
SetProvider#get has odd behavior for null values, we need to handle this
jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/TargetImageParameters.java
Show resolved
Hide resolved
mpeddada1
left a comment
There was a problem hiding this comment.
Added this issue to the CHANGELOG.
Fixes #2760