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

Apply properties from profiles that are active by default #24

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

erwint
Copy link

@erwint erwint commented Apr 28, 2016

When using continuous delivery friendly version numbers with variables,
such variables can be either set externally (-D...) or via a profile
property. While the former works, the later does not. To fix this
properties from profiles that are active by default are now merged into
the effective model.

When using continuous delivery friendly version numbers with variables,
such variables can be either set externally (-D...) or via a profile
property. While the former works, the later does not. To fix this
properties from profiles that are active by default are now merged into
the effective model.
if (profile.getActivation() != null && profile.getActivation().isActiveByDefault()) {
Properties merged = new Properties();
merged.putAll(profile.getProperties());
merged.putAll(model.getProperties());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think, the properties from the model need to be added to merged properties before the properties from the profile. Properties defined in a profile overwrite the ones in the model.

@hohwille
Copy link
Member

Sorry, for the late response - I was totally under water with lots of other OSS and closed projects. Thanks for your PR.
First of all I want to give a short explanation about profiles and flatten-maven-plugin. The idea of this plugin was to create a POM for deployment (mvn install and esp. mvn deploy) that is both minified (aka flattened) but also stable what means your output should not depend on your current environment so a deployment would result in a different POM depending on your OS, env variables, etc.

So far from the theory. In practice you of course can already inject variables via system properties as you pointed out. In the end we can not prevent a user from doing something wrong here.

However, what IMHO should be is that profiles are not triggered by default and this also implies profiles active by default. Why is that? There are tons of maven projects out there that use default profiles for local environment as simplification and use profile triggers to disable them in CI or for deployment builds.
This is why we use properties to activate profile specific behavior in this plugin. So far we only have this one:
http://www.mojohaus.org/flatten-maven-plugin/flatten-mojo.html#embedBuildProfileDependencies

My suggestion would therefore be to add a new property to flatten mojo that is false by default and if activated will trigger the feature that you implemented here. This would also guarantee compatibility when we bring this out in an upcoming version of this plugin (to have reproducible builds even if plugin versions are not specified and upgrading the plugin does not break the behavior in a way where only downgrading brings back the old behavior).

Do you see my point and agree? Would you even be willing to update your fork and this PR accordingly?
Thank you so much for taking care.

@haisum
Copy link

haisum commented Oct 1, 2018

was this feature implemented somewhere else?

@hohwille
Copy link
Member

Not AFAIK. I fear that we create a feature that will cause releases to contain POMs with passwords from settings.xml accidentally.

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

Successfully merging this pull request may close these issues.

4 participants