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

Add support for setting dockerFile and dockerFileDir via maven properties #438

Closed

Conversation

adrianluisgonzalez
Copy link
Contributor

After upgrading to v0.15.1, I see the docker.assembly.dockerFileDir has been deprecated. It would be nice to retain support for configuring via properties.

In addition, when the BuildImageConfiguration is resolved from properties, the initAndValidate() method needs to be called in order for the dockerFileFile and dockerFileMode to be set (docker.assembly.dockerFileDir no longer works).

rhuss added a commit that referenced this pull request May 6, 2016
Refactored a bit the validation handling since it would be otherwise called to often.
@rhuss
Copy link
Collaborator

rhuss commented May 6, 2016

Thanks a lot ! I tuned it a bit, but otherwise everythings fine.

Just out of curiosity: Why do you prefer property based configuration over XML style ?

@adrianluisgonzalez
Copy link
Contributor Author

Great!

We tend to put all our project configuration in properties so it gives us just one place to look rather than multiple locations. I wouldn't say one is better than the other, but it allows greater consistency for how we work. For the same reason we prefer the external Dockerfile to make it more consistent with how we build our other docker images.

@adrianluisgonzalez
Copy link
Contributor Author

@rhuss, I had a quick look at the integration branch and I noticed the following commit after merging this PR:
532529e

The change to src/main/java/io/fabric8/maven/docker/AbstractDockerMojo.java on line 247 removes the following:

resolved.initAndValidate(log);

This means BuildImageConfiguration.initDockerFileFile(Logger log) is not called on the configuration after the properties have been resolved so isDockerFileMode() remains false.

The unit test you added explicitly calls init:

 ImageConfiguration config = resolveExternalImageConfig(testData);
 config.initAndValidate(null);

Maybe I am missing something, or there a better place to trigger the init methods to run? I would have added the initAndValidate(log) call to the ImageConfiguration.Builder.build() method, but there isn't a log instance available.

Hope that makes sense.

@rhuss
Copy link
Collaborator

rhuss commented May 12, 2016

The critical point is this line which already does the resolution of the images before the validation. Hope this makes sense.

@adrianluisgonzalez
Copy link
Contributor Author

@rhuss, thanks for the response. However, I believe there is still a problem.

Each time getImages() is called, new instances of resolved ImageConfiguration are created that have not had initAndValidate() called. Is the intention to keep the instances of resolved images so the same instances that are validated are used?

Something like this:

    private List<ImageConfiguration> resolveImages() {
        if (resolvedImages != null) {
            return resolvedImages;
        }
        List<ImageConfiguration> ret = new ArrayList<>();
        if (images != null) {
            for (ImageConfiguration image : images) {
                ret.addAll(imageConfigResolver.resolve(image, project.getProperties()));
            }
            verifyImageNames(ret);
        }
        return resolvedImages = ret;
    }

I have created a new PR #445

rhuss added a commit that referenced this pull request May 17, 2016
Moved out the resolving, customization, initializaition and validation in an external ConfigurationResolver utility class. resolved images are now cached, so that the initialization is kept.
Todo: Unit tests.
rhuss added a commit that referenced this pull request May 17, 2016
@rhuss
Copy link
Collaborator

rhuss commented May 17, 2016

Thanks, you are right, the resolved images should be cached.

I refactored the initial configuration handling quite a bit and pushed it to branch integration. Maybe you could have a look whether it works for you that way ?

@adrianluisgonzalez
Copy link
Contributor Author

Thanks, the integration branch is working for me. Cheers!

@adrianluisgonzalez adrianluisgonzalez deleted the feature/dockerfile branch June 30, 2016 19:54
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.

2 participants