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

Fixes #957 Simplify configuration #969

Merged
merged 1 commit into from Mar 18, 2018

Conversation

Projects
None yet
2 participants
@rohanKanojia
Copy link
Collaborator

rohanKanojia commented Mar 10, 2018

When only a single image is build with Dockerfile, no need to provide
too much parameters in configuration; build ImageConfiguration dynamically
with the given defaults.
#957

@rohanKanojia rohanKanojia added the WIP label Mar 10, 2018

@rohanKanojia rohanKanojia requested a review from rhuss Mar 10, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 10, 2018

Codecov Report

Merging #969 into master will decrease coverage by 0.17%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##             master    #969      +/-   ##
===========================================
- Coverage     51.57%   51.4%   -0.18%     
  Complexity     1232    1232              
===========================================
  Files           144     144              
  Lines          7251    7276      +25     
  Branches        981     985       +4     
===========================================
  Hits           3740    3740              
- Misses         3195    3220      +25     
  Partials        316     316
Impacted Files Coverage Δ Complexity Δ
...va/io/fabric8/maven/docker/AbstractDockerMojo.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...a/io/fabric8/maven/docker/util/DockerFileUtil.java 63.82% <0%> (-11.18%) 11 <0> (ø)
...c/main/java/io/fabric8/maven/docker/BuildMojo.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...abric8/maven/docker/config/ImageConfiguration.java 43.58% <0%> (-4.3%) 12 <0> (ø)
@rhuss
Copy link
Collaborator

rhuss left a comment

hey, looks good. Thanks !

Some minor change requests (see inline), otherwise looks good.

However, you should consider to add a documentation to the asciidoc, too and update the changelog.md

thanks ...

@@ -34,7 +35,11 @@ protected void executeInternal(ServiceHub hub) throws DockerAccessException, Moj
if (skipBuild) {
return;
}
for (ImageConfiguration imageConfig : getResolvedImages()) {
List<ImageConfiguration> resolvedImages = getResolvedImages();
if(resolvedImages == null || resolvedImages.isEmpty()) {

This comment has been minimized.

@rhuss

rhuss Mar 12, 2018

Collaborator

resolvedImges can't be null otherwise the next for loop would fail with an NPE.

Actually I would either put this into an extra method which returns early in this case or put this into an if-else with the following loop.

@@ -153,6 +154,21 @@ public String initAndValidate(ConfigHelper.NameFormatter nameFormatter, Logger l
return minimalApiVersion;
}

public static ImageConfiguration getDefaultImageConfiguration(MavenProject project, String defaultImageName) {

This comment has been minimized.

@rhuss

rhuss Mar 12, 2018

Collaborator

Can we keep the dependency on Maven low ? actually I would keep the extraction of the default name from maven coordinates in the Mojos.

The reason is, that eventually, we might want to separate the Maven and non-Maven parts (see https://github.com/fabric8io/fabric8-build for more details of these plans).

* @return directory containing the Dockerfile
*/
public static String getDockerFileDirectory() {
if(checkFileExists("./Dockerfile")) {

This comment has been minimized.

@rhuss

rhuss Mar 12, 2018

Collaborator

I would not rely on the current working directory (which might e.g. completely elsewhere in a multi-module project). Instead you ${project.baseDir} (or the MavenProject's equivalent) should be taken into account (which is the absolute path to the module base directory).

*/
public static String getDockerFileDirectory() {
if(checkFileExists("./Dockerfile")) {
return new String(".");

This comment has been minimized.

@rhuss

rhuss Mar 12, 2018

Collaborator

same here: Best to always work with absolute directories, that will work everywhere. With relative directories this is always fragile and it really depends where this method is used.

public static String getDockerFileDirectory() {
if(checkFileExists("./Dockerfile")) {
return new String(".");
} else if(checkFileExists("src/main/docker/Dockerfile")){

This comment has been minimized.

@rhuss

rhuss Mar 12, 2018

Collaborator

dito

if(checkFileExists("./Dockerfile")) {
return new String(".");
} else if(checkFileExists("src/main/docker/Dockerfile")){
return new String("src/main/docker");

This comment has been minimized.

@rhuss

rhuss Mar 12, 2018

Collaborator

what's the difference between new String("src/main/docker") and "src/main/docker" ?

sorry, but that's an oldtimer ;-) and does not make sense at all as String is always immutable. so you can always return the plain string.

Fixes #957 Simplify configuration
+ When only a single image is build with Dockerfile, no need to provide
  too much parameters in configuration; build ImageConfiguration dynamically
  with the given defaults.
+ Updated Changelog.md and asciidoc with related changes

@rohanKanojia rohanKanojia force-pushed the rohanKanojia:issue-957 branch from 0729ec3 to 3a1e6ba Mar 13, 2018

@rhuss

This comment has been minimized.

Copy link
Collaborator

rhuss commented Mar 18, 2018

@rohanKanojia Thanks ! Looks good to me ....

@rhuss rhuss merged commit 689b7e0 into fabric8io:master Mar 18, 2018

2 of 3 checks passed

codecov/patch 0% of diff hit (target 51.57%)
Details
codecov/project 51.4% (-0.18%) compared to 73433ff
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rohanKanojia rohanKanojia removed the WIP label Mar 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment