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

Support standard maven property interpolation #877

Merged
merged 2 commits into from
Nov 4, 2017

Conversation

scoplin
Copy link
Contributor

@scoplin scoplin commented Oct 7, 2017

Replaces the regex-based property interpolation with the plexus classes
used typically by maven to perform interpolation. This allows
references to things like ${project.build.directory} in the Dockerfile.
[fixes #871]

Signed-off-by: Scott Coplin [email protected]

@codecov
Copy link

codecov bot commented Oct 7, 2017

Codecov Report

Merging #877 into master will increase coverage by 0.31%.
The diff coverage is 64.7%.

@@             Coverage Diff             @@
##             master    #877      +/-   ##
===========================================
+ Coverage     50.38%   50.7%   +0.31%     
- Complexity     1196    1203       +7     
===========================================
  Files           139     139              
  Lines          7119    7106      -13     
  Branches        953     952       -1     
===========================================
+ Hits           3587    3603      +16     
+ Misses         3224    3189      -35     
- Partials        308     314       +6
Impacted Files Coverage Δ Complexity Δ
...8/maven/docker/assembly/DockerAssemblyManager.java 12.6% <0%> (+0.15%) 6 <0> (ø) ⬇️
.../io/fabric8/maven/docker/service/BuildService.java 30.3% <0%> (+0.22%) 9 <0> (ø) ⬇️
...a/io/fabric8/maven/docker/util/DockerFileUtil.java 75% <100%> (-2.56%) 11 <3> (-2)
...a/io/fabric8/maven/docker/util/MojoParameters.java 80% <0%> (+5%) 6% <0%> (+1%) ⬆️
...er/assembly/DockerAssemblyConfigurationSource.java 43.67% <0%> (+26.43%) 17% <0%> (+8%) ⬆️

properties.getProperty(prop) :
matcher.group("variable");
matcher.appendReplacement(ret, value.replace("$","\\$"));
else {
Copy link
Contributor

Choose a reason for hiding this comment

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

The else here seems useless, as you're returning in the other branch anyways. It does add a level of indentation though :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Will nix it.

@scoplin
Copy link
Contributor Author

scoplin commented Oct 10, 2017

Hrm, just realized that I missed a few sources of properties that are accounted for on other code paths. Going to take another stab at the properties setup and test cases.

@scoplin scoplin force-pushed the issue_871 branch 2 times, most recently from e9b5621 to 4966022 Compare October 11, 2017 04:05
@scoplin
Copy link
Contributor Author

scoplin commented Oct 11, 2017

So I didn't end up going with a helper method for squashing properties like you suggested in issue #860, @ankon. The main reason was that references like ${project.artifactId} actually appear to be object graph traversals in disguise, and thus not easy to flatten as properties. The practice in the assembly plugin (already a dependency of the docker plugin) seems to be to chain FixedValueSource objects into an interpolator, so that's what I did here.

This commit should only affect interpolation on the Dockerfile and include all the usual sources for interpolation. It doesn't attempt to extend that to other areas where full properties interpolation may or may not be appropriate.

Also, I noted in the docs that the assembly feature is another way to get resource filtering without invoking another plugin since it supports token replacement in files and file sets.

@ankon
Copy link
Contributor

ankon commented Oct 11, 2017

I actually like the idea of the interpolator encapsulating that magic, it's a lot clearer than having a weird static method somewhere!

@rhuss
Copy link
Collaborator

rhuss commented Oct 11, 2017

Thanks a lot ! I hope I have a chance to review today. busy times ...

@scoplin
Copy link
Contributor Author

scoplin commented Nov 2, 2017

Any possibility that this could be reviewed soon @rhuss ?

(Two other open PRs I have that are currently blocking me from switching to fabric8 from the deprecated @alexec docker plugin.)

@rhuss
Copy link
Collaborator

rhuss commented Nov 2, 2017

@scoplin sorry for the delay, the plan is to integrate your (and other PRs) until the beginning of next week and publish a release right after.

Scott Coplin and others added 2 commits November 4, 2017 08:52
Replaces the regex-based property interpolation with the plexus classes
used typically by maven to perform interpolation.  This allows
references to things like ${project.build.directory} in the Dockerfile.
[fixes fabric8io#871]

Signed-off-by: Scott Coplin <[email protected]>
@rhuss
Copy link
Collaborator

rhuss commented Nov 4, 2017

Awesome PR, thank you so much !

Just merged, going through to some other PRs and then make a release today.
sorry again for being late, hope it gets better again in the future ...

@rhuss rhuss merged commit 9eec114 into fabric8io:master Nov 4, 2017
@rhuss
Copy link
Collaborator

rhuss commented Nov 4, 2017

@scoplin 0.23.0 is out. Thanks for your contribution !

@scoplin
Copy link
Contributor Author

scoplin commented Nov 4, 2017

The release version seems to all check out for me. Thanks @rhuss !

@ankon
Copy link
Contributor

ankon commented Nov 6, 2017

I can confirm that 0.23.0 also works for me, thank you all!

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.

dockerFile doesn't do any parameter interpolation
3 participants