Skip to content
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

Ignore empty List<String> configuration values #816

Merged
merged 5 commits into from
Jul 26, 2017
Merged

Ignore empty List<String> configuration values #816

merged 5 commits into from
Jul 26, 2017

Conversation

chonton
Copy link
Contributor

@chonton chonton commented Jul 14, 2017

Allow empty list elements. This often occurs when a parent (corporate) pom has configuration values that are overridden by some, but not all, children.

@codecov
Copy link

codecov bot commented Jul 14, 2017

Codecov Report

Merging #816 into master will increase coverage by 0.06%.
The diff coverage is 92.85%.

@@             Coverage Diff             @@
##             master    #816      +/-   ##
===========================================
+ Coverage     47.53%   47.6%   +0.06%     
  Complexity     1072    1072              
===========================================
  Files           134     134              
  Lines          6927    6936       +9     
  Branches        911     913       +2     
===========================================
+ Hits           3293    3302       +9     
  Misses         3350    3350              
  Partials        284     284
Impacted Files Coverage Δ Complexity Δ
...ic8/maven/docker/config/RunImageConfiguration.java 86.86% <100%> (+0.72%) 36 <1> (ø) ⬇️
...ain/java/io/fabric8/maven/docker/util/EnvUtil.java 65.27% <100%> (+3.05%) 41 <2> (+3) ⬆️
...8/maven/docker/config/BuildImageConfiguration.java 82.71% <75%> (ø) 45 <3> (-1) ⬇️
...bric8/maven/docker/assembly/DockerFileBuilder.java 90.1% <0%> (-1.05%) 72% <0%> (-2%)

Copy link
Collaborator

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Thanks for the PR ! I wonder though whether there is a valid scenario where a null element is valid entry and should not be squeezed out ? Ad-hoc I can't think about one, but if such a use case exists, it would be problematic here.

public List<String> getPorts() {
return ports;
return EnvUtil.removeEmpties(ports);
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor nitpick: Could we rename it to removeNullEntries() ?

Copy link
Contributor Author

@chonton chonton Jul 17, 2017

Choose a reason for hiding this comment

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

A command line equivalent would be docker run -p "" image. This results in the message docker: No port specified: <empty>.

I want to make sure we agree on the distinction between null and empty. When I say null entry, I mean list.get(x) == null; when I say empty entry, I mean list.get(x).isEmpty() == true.

I'm not sure that maven injection can create a null entry. I've definitely seen empty entries; the following produces an empty entry:

<ports>
  <port></port>
</ports>

I'm fine with renaming method. Does removeNullEntries entirely describe the removal of both null and empty values?

As I write this, I wonder if the strings should also have trim() applied?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah. ok. I thought it was about null entries. We should keep it of course like this, maybe calling it 'removeEmptyEntries()' ?

Trim is find for me, too.

Copy link
Collaborator

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Thanks a lot ! One last plea: Could you please update the doc/changelog.md referencing this fix ?

@chonton-elementum
Copy link
Contributor

@rhuss, did you intend to merge this into master or not? Do I need to create another pull request with new base?

@rhuss
Copy link
Collaborator

rhuss commented Jul 26, 2017

Sorry, I simply forgot to merge ;-)

[merge]

@fusesource-ci fusesource-ci merged commit dd2eb44 into fabric8io:master Jul 26, 2017
@chonton chonton deleted the Ignore_Empty_Port_Mappings branch July 30, 2017 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants