maven buildpack environment configuration

[hone]

Context

JAVA_HOME

At Salesforce, we're trying to use the heroku/maven buildpack outside of the standard heroku builder images. Unfortunately, which is not in the base image which causes the mvn binary to fail while it's trying to find the java command:

[builder] [Executing Maven]                                                                                                                                                                                                                                                       
[builder] $ mvn -DskipTests clean install                                                                                                                                                                                                                                         
[builder] /layers/heroku_maven/maven/bin/mvn: line 93: which: command not found 

If JAVA_HOME is set, then mvn does not execute which.

NEXUS Buildpack Compatibility

Secondly, the buildpack today only reads env var from the <platform>/env directory which only captures env vars set by the user or operator. If a prior buildpack sets a build time env var it gets ignored since lifecycle will set this in the current environment. This breaks a nexus buildpack that writes out a settings.xml with MAVEN_SETTINGS_PATH defined.

Changes

This PR introduces two changes that fix the issues for us.

1. In the maven buildpack when reading env vars from <platform>/env also check the current env as a fallback, so user/operator env vars take precedence.
2. Set JAVA_HOME in the env when mvn is executed, so it honors the env var.

TODO

Happy to do these items assuming you're ok with these changes:

• Add tests
• Add changelog

[Malax]

Thank you for the PR! The change to the buildpack logic is something I can get behind - why wouldn't we allow setting buildpack configuration from outside the platform env? 👍🏻 JAVA_HOME is a little bit different, but with the problems you outlined, we should pass that through to Maven too!

Thinking about this more: I'm unsure how much I like the way env vars are handled in general. One one hand, I don't want to just use all env vars that are somehow set to run Maven (or any executable) as that might change the behaviour - regardless where they came from (platform or just regular env).

Maybe some sort of allow listing is the answer here and use both platform and regular env vars as the source.

This is of course out of scope for this PR and I'd be happy to merge it (after resolving the one comment) and tackle the root of the problem later. Especially since the env var behaviour should be consistent across all of our buildpacks.

[hone]

JAVA_HOME is a little bit different, but with the problems you outlined, we should pass that through to Maven too!

Yeah, Java being on the base image and just being able to the maven buildpack seems like a pretty good use case to me.

Thinking about this more: I'm unsure how much I like the way env vars are handled in general. One one hand, I don't want to just use all env vars that are somehow set to run Maven (or any executable) as that might change the behaviour - regardless where they came from (platform or just regular env).

Maybe some sort of allow listing is the answer here and use both platform and regular env vars as the source.

Yeah, that makes sense to me. Hopefully, this reduces the impact radius since it's only those 3 maven env vars like before it just sources it from more than one source now.

Especially since the env var behaviour should be consistent across all of our buildpacks.

Yeah, I didn't want to try to tackle this at that level yet since probably more discussion to be had and probably don't want to duplicate this code everywhere. It seems like a bug to not read from both places in a lot of cases especially given that operators can specify env vars at build time that will end up there, RFC.

[Malax]

Yeah, Java being on the base image and just being able to the maven buildpack seems like a pretty good use case to me.

Just wondering, how does this pass the check for jvm in the build plan? Did you add a bogus buildpack to the order to make sure that requirement is satisfied?

It seems like a bug to not read from both places in a lot of cases especially given that operators can specify env vars at build time that will end up there, RFC.

I think I agree with it being a bug. I'll think about this some more how we can make this more convenient for buildpack authors.

[hone]

Just wondering, how does this pass the check for jvm in the build plan? Did you add a bogus buildpack to the order to make sure that requirement is satisfied?

I personally did not write it, but yes there is a buildpack that does nothing during build and just provides/requires jdk during detect.