-
Notifications
You must be signed in to change notification settings - Fork 85
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
Fix Profile Interpolation bug #114
Conversation
… profiles section interpolated if profile is active during build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ciis0 Thanks for this PR 👍
I would like to first discuss whether we should "fix" the existing resolveCiFriendliesOnly
as it seems to be quite buggy or create a new mode as suggested by this PR.
Do not get me wrong. I like that you care about existing behaviours that you do not want to break.
Also I would like to integrate your work and pull out a new release with this fix.
However, I do not yet understand the full context and want to take the right decision.
@@ -15,7 +15,7 @@ | |||
<groupId>org.codehaus.mojo</groupId> | |||
<artifactId>flatten-maven-plugin</artifactId> | |||
<configuration> | |||
<flattenMode>resolveCiFriendliesOnly</flattenMode> | |||
<flattenMode>version</flattenMode> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not missuse existing ITs to test a new feature. You would need to create a new integration test instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO we should either "fix" the existing mode, or if we keep it create a new IT for version
mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per #114 (comment) resolveCiFriendlies
should be fixed. :)
/** | ||
* Fix for {@link #resolveCiFriendliesOnly} | ||
*/ | ||
version; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if this is a "fix" for resolveCiFriendliesOnly
why do we have to introduce a new mode then?
My impression is also that this plugin can not be understood by an avergage user anymore. So what does version
mode do? Where is it documentated? I already had concerns like this with the naming of resolveCiFriendliesOnly
and the documentation is already lacking there but I did not want to be a blocker for urgently requested features. I think that your PR is in general providing a very helpful improvement as I also experienced some problems/bugs with this resolveCiFriendliesOnly
. However the way we address this should IMHO need some improvement.
@lasselindqvist do you have some cents on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just kept it during rebase, no other reasons from my side. ^^
/** Take the element untouched from the original POM. Fix for {@link #keep} */ | ||
keepRaw |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting. So actually keep
does not actually do what it should? Can you give an example?
And you invented a new handling to avoid breaking existing build configs?
That would make a lot of sense. So does keep
somehow already resolve variables or what is it that it does too much?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem that arose for me was that the configuration in (active) profiles was interpolated, specifically ${basedir}
.
And I did not invent anything, just rebased it. ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without wanting to be rude, but the overall direction of the original PR-code is rather being a work-around that being a real fix :D
I guess the work remaining now is to fix the underlying issues so the workarounds (version
, keepRaw
) are not necessary anymore.
I basically invented nothing here, just rebased it :D I just kept the original new In the context I encounter the bug in I'd say this kind of behaviour is not worth keeping backwards-compatible, it simply does the wrong thing in a way that and. |
Although the underlying code is not mine, I am motivated to fix the issues that arose; the bug is really annoying in some of our projects :D |
See also comments in: #103 I don't think this is in a mergeable state, as it contains a mix of commits for different things, but the integration test idea is definitely usable here and opening a PR with two commits: the first introducing the test, and the second introducing the fix should be merged. The added test has
but I think we want to have something like
so that we see that the revision is resolved in the profile, but other variables are not. |
Checked this behavior and the problem is that the profile arrives to FlattenMojo.createResolvedPom already resolved to Seems like this issue exists regardless of the resolveCiFriendliesOnly flatten mode, which might make it harder to solve. |
Yup, that's the |
#126 supersedes this PR. |
#102 rebased
Closes #102
Fixes #103