[JENKINS-20884] Variable expansion in maven goals #14
Conversation
|
plugins » maven-plugin #46 UNSTABLE |
|
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
|
can you please add a test case? e.g. along the same way as it was made here jenkinsci/jenkins#799 |
|
I've created a test. But it is not working yet. I have a problem with prebuilders, because they don't populate actions on getRootBuild() object, which I use in my changed code, in MavenModuleSetBuild. Perhaps you could look into the unit test and advise something? |
|
plugins » maven-plugin #47 FAILURE |
|
please have a go with the PR I have just placed ( #16 ) - this might fix your issue with the testcase too |
|
👍 this would be very very... useful |
|
Any updates? |
|
|
||
| for (Action action : getRootBuild().getActions()) { | ||
| if(action instanceof EnvironmentContributingAction){ | ||
| ((EnvironmentContributingAction) action).buildEnvVars(MavenModuleSetBuild.this, envVars); |
There was a problem hiding this comment.
Normally this should not have to be called from plugins, because AbstractBuild.getEnvironment(TaskListener) calls it already.
There was a problem hiding this comment.
AbstractBuild.getEnvironment(TaskListener) is called before prebuilders, and groovy or other prebuilder may modify in EnvironmentContributingAction some values. So envVars need to be refreshed
There was a problem hiding this comment.
Calling again envVars.overrideExpandingAll(getEnvironment(listener)) is potentially wrong. Is there any way to keep EnvVars transient so prebuilds had ability to access and modify it directly instead of polling classes?
There was a problem hiding this comment.
Also prebuilders like Groovy script calls .getEnvironment(listener) from perform internals and uses EnvVars that maybe not the same that were called in MavenModuleSetBuild. If there is no critical overrides in variables before, then additional call to .getEnvironment(TaskListener) maybe a solution.
…ates a bad precedent. It is better to just recompute the envVars, which will reflect all the added environments. See: #14 Relevant conversation: (11:31:53 AM) KostyaSha: kohsuke, while windows is eating your IO, could you advice with #14 ? (11:34:27 AM) kohsuke: KostyaSha: the bug looks legit, but I'm not sure about the fix. Basically I agree with Jesse's comment (11:35:35 AM) KostyaSha: kohsuke, this broken by design i think, it should be a good idea to share envvars for builders. This should also reduce .getEnvironments() calls (11:36:33 AM) KostyaSha: kohsuke, is there any place where they maybe safely shared? (11:37:03 AM) kohsuke: I'm afraid I don't understand the notion of "sharing envvars" (11:37:35 AM) kohsuke: It gets computed from various things, and I thought EnvironmentContributingAction is a part of it (11:38:44 AM) KostyaSha: kohsuke, yes, and they are stored in envvars variable, then job calls prebuilders that modifies EnvironmentContributingAction and then job calls maven build with not updated envvars content (11:39:05 AM) kohsuke: then it should just call EnvVars envVars = getEnvironment(listener); again (11:39:50 AM) KostyaSha: kohsuke, i not sure that there is no any specific changes with envvars after first getEnvironment(listener) call and before prebuilders (11:40:30 AM) KostyaSha: i compared on my local instance and this should work, but i not sure... potentially some changes to envvars maybe lost... (11:40:31 AM) kohsuke: Basically, one should never modify what Run.getEnvironment() returned (11:40:51 AM) kohsuke: If the map returned is missing some desirable entries, then it should be fixing by having the getEnvironment() implementation add them (11:41:11 AM) kohsuke: It looks to me that this change violates that idea (11:42:02 AM) KostyaSha: i like idea of refreshing with simple call to getEnvironment(listener) (11:42:03 AM) kohsuke: if environment-contributing subset of rootBuild.actions should be a part of the env vars, according to the above principle it should be done in the getEnvironment() method (11:42:52 AM) kohsuke: (Also, let's not print out random stuff into "logger.println" that most users would not care (11:43:02 AM) kohsuke: Those should be j.u.logging statements) (11:43:28 AM) KostyaSha: kohsuke, yeah this logger not needed of course (11:44:55 AM) KostyaSha: kohsuke, https://github.com/zygm0nt/maven-plugin/blob/master/src/main/java/hudson/maven/MavenModuleSetBuild.java#L648 what this part do? (11:45:44 AM) kohsuke: That looks wrong to me, too, for the same reason (11:45:49 AM) kohsuke: All right, you convinced me to open this in IDE (11:46:03 AM) kohsuke: Screw the preparation for the meeting (11:47:29 AM) kohsuke: Wow, that line has jglick fixing HUDSON-3502 (11:47:31 AM) jenkins-admin: JENKINS-3502:Xvnc does not set the DISPLAY environment (Closed) https://issues.jenkins-ci.org/browse/JENKINS-3502 (11:48:06 AM) KostyaSha: 0_o (11:48:17 AM) kohsuke: From 2009 (11:49:34 AM) KostyaSha: kohsuke, and the next block is also added for resolving variables (11:49:42 AM) kohsuke: Yep (11:50:28 AM) KostyaSha: so after every step we need recalculate changes... so easy to allow do direct modifications of envvars i think
|
I agree with Jesse, in that it looks wrong to have My fix from cab3151 to 14f28ed addresses this accordingly, and it passes the test @zygm0nt wrote. |
I had a Jenkins job, where I needed to calculate goals to be executed in a shell script. So I two pre-build steps:
Then I wanted to put that variable into maven goals, like this:
This did not work. This change fixes that.
Would be happy to know if this is ok, or should be fixed in some other way.
Regards
Marcin