[MNG-8081] interpolate available properties during default profile selection (Maven 4.x)

[mbenson]

Evaluate profile activations after having interpolated embedded property constructs.

Following this checklist to help us incorporate your contribution quickly and easily:

• [X] Make sure there is a JIRA issue MNG-8081 filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
• [X] Each commit in the pull request should have a meaningful subject line and body.
• [X] Format the pull request title like [MNG-XXX] SUMMARY, where you replace MNG-XXX and SUMMARY with the appropriate JIRA issue.
• [X] Also format the first line of the commit message like [MNG-XXX] SUMMARY. Best practice is to use the JIRA issue title in both the pull request title and in the first line of the commit message.
• [X] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• [X] Run mvn clean verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
• [X] You have run the Core IT successfully.

If your pull request is about ~20 lines of code you don't need to sign an Individual Contributor License Agreement if you are unsure please ask on the developers list.

To make clear that you license your contribution under the Apache License Version 2.0, January 2004 you have to acknowledge this by using the following check-box.

• [X] I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

• [X] In any other case, please file an Apache Individual Contributor License Agreement.

ICLA on file

[mbenson]

Second iteration:

• test case fails without updated code
• using MavenTransformer for interpolation

[gnodet]

The code looks good.

However, what's the real use case for that ? It seems actually unrelated to MNG-5235, in a sense that the behaviour on this project is not modified by this PR. My understanding is that the use case for MNG-5235 is to activate profile based on project properties, while this PR is about interpolating activation property values (but not based on project properties). Also note that properties defined from activated external profiles are already taken into account, see MNG-2276. Should this require a different JIRA issue ? I'm not really sure to understand the use case, specifically why you'd want the activation part to possibly change from a maven run to another ?

[mbenson]

My use case is to be able to compare environment variables. Specifically, with GitLab CI, if CI_COMMIT_REF_NAME has the same value as CI_DEFAULT_BRANCH, then I want to enable stricter enforcer checks. For my purposes I'd rather the project make assumptions about the CI than vice versa.

[gnodet]

My use case is to be able to compare environment variables. Specifically, with GitLab CI, if CI_COMMIT_REF_NAME has the same value as CI_DEFAULT_BRANCH, then I want to enable stricter enforcer checks. For my purposes I'd rather the project make assumptions about the CI than vice versa.

Not sure how you'll model that with the limited support we have right now, do you ? Maybe we should extend the model activation to support a simple language which provide basic operations such as = and !=. Something like:

<property>
  <expression>${env. CI_COMMIT_REF_NAME} = ${env. CI_DEFAULT_BRANCH}</expression>
</property>

At some point, it may be easier to provide a custom Maven extension that would compute a system property based on those two variables so you can easily activate a profile based on the computed property. Maven 4 added org.apache.maven.api.spi.PropertyContributor for such use case (to have a cleaner api, but it's possible in Maven 3 using the AbstractMavenLifecycleParticipant. See https://github.com/trustin/os-maven-plugin/tree/master and https://github.com/trustin/os-maven-plugin/pull/74 An example

[mbenson]

Hmm, my approach was, e.g.:

<activation>
  <property>
    <name>env.CI_COMMIT_REF_NAME</name>
    <value>${env.CI_DEFAULT_BRANCH}</value>
  </property>
</activation>

...which seems fine to me 🤷

[rmannibucau]

personally i do it in the sh part of the yaml:

case ${DEPLOY_ENV:-dev} in
  dev)
    export SCRIPTS_DEPLOY_DEPLOYMENT_PROFILES="-Pshared-dev -Dmonitoring.enabled=true"
    export SCRIPTS_DEPLOY_DEPLOYMENT_ALVEOLUS="minikube"
    ;;
  *)
    export SCRIPTS_DEPLOY_DEPLOYMENT_PROFILES="-P$DEPLOY_ENV"
    export SCRIPTS_DEPLOY_DEPLOYMENT_ALVEOLUS="${SCRIPTS_DEPLOY_DEPLOYMENT_ALVEOLUS:-all-in-one}"
    ;;
esac

# this shared script just runs mvn with the SCRIPTS_DEPLOY_... options but the magic literally happens in previous case and is open bar on the CI
/usr/local/bin/deploy.sh

No assumption needed all is detected in the build pipeline (where it belongs) and project itself just enables to activate it or not.

[mbenson]

@rmannibucau but your pipeline has to know/hope that the project has such a shared-dev profile. For my purposes my pipeline wouldn't know what such a profile might be named, but my org's projects do know what CI they run on (in the worst case, migrating to a different CI--which won't actually happen, but if it did--it would be pretty easy to make XXX CI emulate GitLab CI by setting the same environment variables).

[gnodet]

Hmm, my approach was, e.g.:

<activation>
  <property>
    <name>env.CI_COMMIT_REF_NAME</name>
    <value>${env.CI_DEFAULT_BRANCH}</value>
  </property>
</activation>

...which seems fine to me 🤷

Yes, obviously that'll work 😊

[rmannibucau]

@mbenson see it the other way, if (magic) -DdoItSlow then you can use this new fake variable accordingly. You can migrate but the easiest is a custom base image IMHO which does this for you and abstracts this logic.

[mbenson]

@rmannibucau I think it's okay if different users want to interact with Maven in different ways. Your preference does not invalidate mine.

[mbenson]

@gnodet I agree that MNG-5235 has a different ask than this PR addresses. I will open a new issue and rewrite this commit message accordingly (pushed to same branch name to keep using this PR).

[rmannibucau]

@mbenson well a mini language got rejected multiple times on the list - this is where this sh solution comes from. ultimately I'm for having a multipass filtering to filter as much as we can but my personal preference would be to do it on the full model and not specifically for profiles. That said, while v4 has the exact same code than v3.9 I'm happy.

[gnodet]

@mbenson well a mini language got rejected multiple times on the list - this is where this sh solution comes from. ultimately I'm for having a multipass filtering to filter as much as we can but my personal preference would be to do it on the full model and not specifically for profiles. That said, while v4 has the exact same code than v3.9 I'm happy.

It seems it can be solved really easily though: https://github.com/kpiwko/el-profile-activator-extension/blob/master/src/main/java/com/redhat/jboss/maven/elprofile/ElProfileActivator.java

[gnodet]

@mbenson well a mini language got rejected multiple times on the list - this is where this sh solution comes from. ultimately I'm for having a multipass filtering to filter as much as we can but my personal preference would be to do it on the full model and not specifically for profiles. That said, while v4 has the exact same code than v3.9 I'm happy.

It seems it can be solved really easily though: https://github.com/kpiwko/el-profile-activator-extension/blob/master/src/main/java/com/redhat/jboss/maven/elprofile/ElProfileActivator.java

That makes we wonder if this PR targets the wrong location. Should we modify the PropertyProfileActivator instead ?

[mbenson]

That was what I originally considered, but I quickly came to the opinion that as soon as we did it that way, someone would want to use some such property to match a Java version, etc., and that this would be a generally applicable way to solve a broad cross section of similar use cases.

[gnodet]

That was what I originally considered, but I quickly came to the opinion that as soon as we did it that way, someone would want to use some such property to match a Java version, etc., and that this would be a generally applicable way to solve a broad cross section of similar use cases.

How could that (solving similar use cases) be considered a bad thing ?

[mbenson]

That was what I originally considered, but I quickly came to the opinion that as soon as we did it that way, someone would want to use some such property to match a Java version, etc., and that this would be a generally applicable way to solve a broad cross section of similar use cases.

How could that (solving similar use cases) be considered a bad thing ?

That's what I wanted to know 😮 i.e., I think this approach takes care of them in one place before everybody comes asking for similar changes to be made everywhere.

[mbenson]

From email thread:

Guillaume Nodet [email protected] wrote:

I thought you were referring to having a small language in the property activation...

I hadn't noted the discussion when it took place before, but fwiw I also lean in the direction that an embedded language might be going too far. At the same time EL, as in the extension you mentioned, seems a fine choice, but feels fine to leave that as an extension.

Matt

[gnodet]

From email thread:

Guillaume Nodet [email protected] wrote:

I thought you were referring to having a small language in the property activation...

I hadn't noted the discussion when it took place before, but fwiw I also lean in the direction that an embedded language might be going too far. At the same time EL, as in the extension you mentioned, seems a fine choice, but feels fine to leave that as an extension.

Matt

@mbenson I just spotted that the model builder performs some interpolation for file base activation in https://github.com/apache/maven/blob/5029cc238ce5ce03b0fb431a278889f8b788c0a7/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java#L896-L943 Would it make sense to move this code in this block to interpolate ? It seems more intuitive that the two interpolations would actually be performed at the same time...

[mbenson]

That is possible, sure. I'll take a look in a bit.

UPDATE: Yes, if there is already an "interpolate activations" activity taking place in the model builder, I agree it makes sense to add the expanded functionality there. In an early iteration of this work I encountered test failures relating to file paths when I allowed the interpolations to remain in the model, but reviewing this section of the code I see that that probably related to the relative path computation that's being done for file activations, so hopefully by taking that into consideration I can avoid such issues this time around.

[mbenson]

Here is the version that enhances DefaultModelBuilder. Can anyone offer ideas on the likely shape of unit tests for this approach?

[mbenson]

By way of [some] testing, I went ahead and expanded the profile activation model validation to consider the whole activation, similarly to how we are now interpolating the whole activation.

Here I could be construed as misusing the MavenTransformer to walk the activation; if this is deemed offensive we could consider generating a similar MavenWalker class.

[mbenson]

Thanks @gnodet and @rmannibucau for the approvals! What is the next step?

[gnodet]

Thanks @gnodet and @rmannibucau for the approvals! What is the next step?

I'm nearly done with http://github.com/apache/maven/pull/1457, so as soon as that one is merged, we'll be able to port this PR to the new model builder easily.

[mbenson]

@gnodet do you have any thoughts on the best way to backport this feature to Maven 3? https://github.com/apache/maven/pull/1447 is my original PR, but a wildly divergent implementation may cause headaches in future. I wonder if it would make sense [from a future merge perspective] to stabilize the v3 impl and include the same commit in this branch prior to being replaced by the actual v4 implementation. 🤔

[gnodet]

@mbenson I've rebased the PR and ported it to the new v4 model builder, please have a look

[mbenson]

hi @gnodet ... so with the implementation having been largely duplicated from model-builder into api-impl, would the similar code remain in both places indefinitely, or will the model-builder code eventually point directly to api-impl?

[gnodet]

hi @gnodet ... so with the implementation having been largely duplicated from model-builder into api-impl, would the similar code remain in both places indefinitely, or will the model-builder code eventually point directly to api-impl?

The code in model-builder should be deprecated, it will be kept for compatibility with 3.x plugins for some time, and then removed. Not sure about the exact dates though. The point is that model-builder is not really used anymore in beta-1.

[mbenson]

Incorporated noneMatch as suggested. IDK how I missed that pair of Stream methods all this time. 😕

[mbenson]

@gnodet again, thanks!