Skip to content
Permalink
Browse files
Prevent collisions in env vars when using multiple SCMs of the same kind
[FIXED JENKINS-30709]
  • Loading branch information
JakeStoeffler authored and rodrigc committed Apr 3, 2016
1 parent f0088b6 commit 2490dd7e48cf485f91797a14a5e2a700848ae76a
Showing with 16 additions and 2 deletions.
  1. +16 −2 src/main/java/org/jenkinsci/plugins/multiplescms/MultiSCM.java
@@ -1,5 +1,6 @@
package org.jenkinsci.plugins.multiplescms;

import hudson.EnvVars;
import hudson.Extension;
import hudson.FilePath;
import hudson.Launcher;
@@ -30,6 +31,7 @@
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import net.sf.json.JSONArray;

import net.sf.json.JSONObject;
@@ -71,12 +73,24 @@ public SCMRevisionState calcRevisionsFromBuild(AbstractBuild<?, ?> build,

@Override
public void buildEnvVars(AbstractBuild<?,?> build, Map<String, String> env) {
for(SCM scm : scms) {
// Add each SCM's env vars, appending indices where needed to avoid collisions
for (int i = 0; i < scms.size(); i++) {

This comment has been minimized.

Copy link
@Sauraus

Sauraus Apr 14, 2016

Don't count from 0, start 1 and go up, because that way you can retain the current behaviour of the last SCM entry being without a _X tag on it and thereby not break hundreds of job configs out there that depend on this.

try {
scm.buildEnvVars(build, env);
EnvVars currScmVars = new EnvVars();
scms.get(i).buildEnvVars(build, currScmVars);
for (Entry<String, String> entry : currScmVars.entrySet()) {
if (env.containsKey(entry.getKey())) {
// We have a collision; append the index of this SCM to the env var name
env.put(entry.getKey() + "_" + i, entry.getValue());
} else {
// No collision; just put the var as usual

This comment has been minimized.

Copy link
@Sauraus

Sauraus Apr 14, 2016

Don't do this, keep it consistent, everything for SCM_1 must go into SCM_1 do not assume it is generic.

env.put(entry.getKey(), entry.getValue());
}
}
}
catch(NullPointerException npe)
{}

This comment has been minimized.

Copy link
@Sauraus

Sauraus Apr 14, 2016

Never ever do a blind catch, this is just begging for trouble!


}
}

8 comments on commit 2490dd7

@Sauraus

This comment has been minimized.

Copy link

@Sauraus Sauraus replied Apr 14, 2016

Sorry for the harsh comments, but this change broke several pipelines... with no warning.

@JakeStoeffler

This comment has been minimized.

Copy link
Contributor Author

@JakeStoeffler JakeStoeffler replied Apr 14, 2016

@Sauraus Feel free to fix those issues... Like I hinted at in the PR, this probably needed some review & cleanup before being merged in since this is my first time contributing to Jenkins. I didn't even realize that it had been merged into master.

@willneumob

This comment has been minimized.

Copy link

@willneumob willneumob replied Apr 14, 2016

This isn't working for me with 0.7-SNAPSHOT btw. If I I set the build script to simply env, I still only see GIT_COMMIT, although I'm using Multiple SCMs with 4 repositories configured. I see GIT_URL_1 through GIT_URL_4, but only one GIT_COMMIT and one GIT_PREVIOUS_COMMIT.

@Sauraus

This comment has been minimized.

Copy link

@Sauraus Sauraus replied Apr 14, 2016

That makes sense, because the code above only creates _1/2/3/4 ENV vars for keys that are non-unique, unique keys do not get _1/2/3/4 appended to them.

If you merge my pull request into your fork then all ENV vars are appended a _1/2/3/4 irrespective of whether they are unique or not. #19

@willneumob

This comment has been minimized.

Copy link

@willneumob willneumob replied Apr 14, 2016

Ah I see. I'll try it out now with your #19 and report back. Thanks!

@willneumob

This comment has been minimized.

Copy link

@willneumob willneumob replied Apr 14, 2016

Ok @Sauraus, so I tested it out, and now we have GIT_COMMIT and GIT_COMMIT_1, but no .._2 ... .._4 :(

@Sauraus

This comment has been minimized.

Copy link

@Sauraus Sauraus replied Apr 14, 2016

See my other comment, based on the code it simply means that there are no other GIT_COMMIT entries for multi SCM plugin to pick up.

@willneumob

This comment has been minimized.

Copy link

@willneumob willneumob replied Apr 14, 2016

This might be a separate bug then. There seems to be no way at all for Jenkins to pull commit hashes for all configured repositories for a build project.

Please sign in to comment.