Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-20884

Variable expansion in maven goals

    Details

    • Similar Issues:

      Description

      Hi

      I had a Jenkins job, where I needed to calculate goals to be executed in a shell script. So I two pre-build steps:

      • shell script
      • inject environment variables (EnvInject)

      Then I wanted to put that variable into maven goals, like this:

      -o clean install -pl ${some_variable}
      

      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

        Attachments

          Issue Links

            Activity

            Hide
            marcin_cylke Marcin Cylke added a comment -
            Show
            marcin_cylke Marcin Cylke added a comment - I've created a pull request: https://github.com/jenkinsci/maven-plugin/pull/14
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Kohsuke Kawaguchi
            Path:
            src/main/java/hudson/maven/AbstractMavenBuild.java
            src/main/java/hudson/maven/MavenModuleSetBuild.java
            http://jenkins-ci.org/commit/maven-plugin/cab3151ea023536ab789f3b74a891f38144fe302
            Log:
            JENKINS-20884 Mutating 'envVars' returned from getEnvironment() creates a bad precedent.

            It is better to just recompute the envVars, which will reflect all the added environments.

            See: https://github.com/jenkinsci/maven-plugin/pull/14

            Relevant conversation:

            (11:31:53 AM) KostyaSha: kohsuke, while windows is eating your IO, could you advice with https://github.com/jenkinsci/maven-plugin/pull/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

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Kohsuke Kawaguchi Path: src/main/java/hudson/maven/AbstractMavenBuild.java src/main/java/hudson/maven/MavenModuleSetBuild.java http://jenkins-ci.org/commit/maven-plugin/cab3151ea023536ab789f3b74a891f38144fe302 Log: JENKINS-20884 Mutating 'envVars' returned from getEnvironment() creates a bad precedent. It is better to just recompute the envVars, which will reflect all the added environments. See: https://github.com/jenkinsci/maven-plugin/pull/14 Relevant conversation: (11:31:53 AM) KostyaSha: kohsuke, while windows is eating your IO, could you advice with https://github.com/jenkinsci/maven-plugin/pull/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
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Marcin Cylke
            Path:
            src/test/java/hudson/maven/MavenEnvironmentContributingActionFromBuilderTest.java
            http://jenkins-ci.org/commit/maven-plugin/4d63a4385502e2c6a98f79880348cf7a0577b98f
            Log:
            JENKINS-20884 Test case contributed in pull-14

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Marcin Cylke Path: src/test/java/hudson/maven/MavenEnvironmentContributingActionFromBuilderTest.java http://jenkins-ci.org/commit/maven-plugin/4d63a4385502e2c6a98f79880348cf7a0577b98f Log: JENKINS-20884 Test case contributed in pull-14
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Kohsuke Kawaguchi
            Path:
            src/test/java/hudson/maven/MavenEnvironmentContributingActionFromBuilderTest.java
            http://jenkins-ci.org/commit/maven-plugin/14f28ed5bff96bd103ea87fc4b31b8995444bc31
            Log:
            JENKINS-20884 Adjusted the test case

            The situation cited in the bug report involves build action, not a
            project action.

            Compare: https://github.com/jenkinsci/maven-plugin/compare/eceb0289ce3a...14f28ed5bff9

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Kohsuke Kawaguchi Path: src/test/java/hudson/maven/MavenEnvironmentContributingActionFromBuilderTest.java http://jenkins-ci.org/commit/maven-plugin/14f28ed5bff96bd103ea87fc4b31b8995444bc31 Log: JENKINS-20884 Adjusted the test case The situation cited in the bug report involves build action, not a project action. Compare: https://github.com/jenkinsci/maven-plugin/compare/eceb0289ce3a...14f28ed5bff9
            Hide
            kohsuke Kohsuke Kawaguchi added a comment -

            Fixed toward 2.7. See the pull request for more discussion.

            Show
            kohsuke Kohsuke Kawaguchi added a comment - Fixed toward 2.7. See the pull request for more discussion.
            Hide
            integer Kanstantsin Shautsou added a comment -

            Tested with groovy injection - works fine.

            Show
            integer Kanstantsin Shautsou added a comment - Tested with groovy injection - works fine.

              People

              • Assignee:
                kohsuke Kohsuke Kawaguchi
                Reporter:
                marcin_cylke Marcin Cylke
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: