Skip to content

Expand environment variables in repository URL. #38

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kutzi merged 11 commits into from
Oct 7, 2013
Merged

Expand environment variables in repository URL. #38

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e06583f
Expand environment variables in repository URL in job confguration du…
jglick Mar 1, 2013
aa9f9d3
$SVN_REVISION also needs to take into account possibly substituted se…
jglick Mar 18, 2013
ba34e12
checkRevisionPropertiesSupported also needs to subst env vars.
jglick Mar 18, 2013
7c0c0fe
Different overload now being called.
jglick Mar 18, 2013
7785a9a
Merge branch 'master' into doCheckRemote-masterEnvVars
jglick May 24, 2013
add273b
Merge branch 'master' into doCheckRemote-masterEnvVars
jglick Jun 6, 2013
eb0edaf
Revised to only use masterEnvVars from getExpandedLocation(AbstractBu…
jglick Jun 6, 2013
65fe472
Merge branch 'master' into doCheckRemote-masterEnvVars
jglick Jun 17, 2013
4b120f4
Make buildEnvVars take into account non-build variables when computin…
jglick Jun 17, 2013
6819927
Merge branch 'master' into doCheckRemote-masterEnvVars
jglick Jul 16, 2013
5e006e5
Make sure SVN_* variables are set even when SVN location involves rec…
jglick Jul 16, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 15 additions & 38 deletions src/main/java/hudson/scm/SubversionSCM.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,9 +403,9 @@ public ModuleLocation[] getLocations(AbstractBuild<?,?> build) {
/**
* List of all configured svn locations, expanded according to all env vars
* or, if none defined, according to only build parameters values.
*
* Both may be defined, in which case the variables are combined.
* @param env If non-null, variable expansions are performed against these vars
* @param build If non-null (and if env is null), variable expansions are
* @param build If non-null, variable expansions are
* performed against the build parameters
*/
public ModuleLocation[] getLocations(EnvVars env, AbstractBuild<?,?> build) {
Expand All @@ -430,15 +430,13 @@ public ModuleLocation[] getLocations(EnvVars env, AbstractBuild<?,?> build) {
return locations;

ModuleLocation[] outLocations = new ModuleLocation[locations.length];
if(env != null) {
for (int i = 0; i < outLocations.length; i++) {
outLocations[i] = locations[i].getExpandedLocation(env);
}
EnvVars env2 = env != null ? new EnvVars(env) : new EnvVars();
if (build != null) {
env2.putAll(build.getBuildVariables());
}
else {
for (int i = 0; i < outLocations.length; i++) {
outLocations[i] = locations[i].getExpandedLocation(build);
}
EnvVars.resolve(env2);
Copy link
Copy Markdown
Member

@kutzi kutzi Oct 5, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to put the recursive into ModuleLocation#getExpandedLocation(EnvVars)?

Copy link
Copy Markdown
Member Author

@jglick jglick Oct 7, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be possible. On the other hand, this would be less efficient—would need to make a defensive copy of the env argument, and the logic would be needlessly repeated for each location. Also this would make a behavioral change to a lower-level method which is only apparently called from here. So unless there is a particular use case that requires the call to resolve to be pushed down, I think it makes more sense here.

Copy link
Copy Markdown
Member

@kutzi kutzi Oct 7, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, makes sense

for (int i = 0; i < outLocations.length; i++) {
outLocations[i] = locations[i].getExpandedLocation(env2);
}

return outLocations;
Expand Down Expand Up @@ -604,7 +602,7 @@ public boolean isFilterChangelog() {
public void buildEnvVars(AbstractBuild<?, ?> build, Map<String, String> env) {
super.buildEnvVars(build, env);

ModuleLocation[] svnLocations = getLocations(build);
ModuleLocation[] svnLocations = getLocations(new EnvVars(env), build);

try {
Map<String,Long> revisions = parseSvnRevisionFile(build);
Expand Down Expand Up @@ -1961,6 +1959,8 @@ public FormValidation doCheckRemote(StaplerRequest req, @AncestorInPath Abstract

if(isValidateRemoteUpToVar()) {
url = (url.indexOf('$') != -1) ? url.substring(0, url.indexOf('$')) : url;
} else {
url = new EnvVars(EnvVars.masterEnvVars).expand(url);
}

if(!URL_PATTERN.matcher(url).matches())
Expand Down Expand Up @@ -2175,7 +2175,7 @@ public FormValidation doCheckRevisionPropertiesSupported(@AncestorInPath Abstrac
return FormValidation.ok();

try {
SVNURL repoURL = SVNURL.parseURIDecoded(v);
SVNURL repoURL = SVNURL.parseURIDecoded(new EnvVars(EnvVars.masterEnvVars).expand(v));
if (checkRepositoryPath(context,repoURL)!=SVNNodeKind.NONE)
// something exists
return FormValidation.ok();
Expand Down Expand Up @@ -2398,31 +2398,6 @@ public SVNRevision getRevision(SVNRevision defaultValue) {
return revision != null ? revision : defaultValue;
}

private String getExpandedRemote(AbstractBuild<?,?> build) {
String outRemote = remote;

ParametersAction parameters = build.getAction(ParametersAction.class);
if (parameters != null)
outRemote = parameters.substitute(build, remote);

return outRemote;
}

/**
* @deprecated This method is used by {@link #getExpandedLocation(AbstractBuild)}
* which is deprecated since it expands variables only based
* on build parameters.
*/
private String getExpandedLocalDir(AbstractBuild<?,?> build) {
String outLocalDir = getLocalDir();

ParametersAction parameters = build.getAction(ParametersAction.class);
if (parameters != null)
outLocalDir = parameters.substitute(build, getLocalDir());

return outLocalDir;
}

/**
* Returns the value of remote depth option.
*
Expand Down Expand Up @@ -2450,7 +2425,9 @@ public boolean isIgnoreExternalsOption() {
* to be performed on all env vars rather than just build parameters.
*/
public ModuleLocation getExpandedLocation(AbstractBuild<?, ?> build) {
return new ModuleLocation(getExpandedRemote(build), getExpandedLocalDir(build));
EnvVars env = new EnvVars(EnvVars.masterEnvVars);
env.putAll(build.getBuildVariables());
return getExpandedLocation(env);
}

/**
Expand Down
21 changes: 21 additions & 0 deletions src/test/java/hudson/scm/SubversionSCMTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,8 @@
import com.gargoylesoftware.htmlunit.html.HtmlAnchor;
import com.gargoylesoftware.htmlunit.html.HtmlForm;
import com.gargoylesoftware.htmlunit.html.HtmlPage;
import hudson.EnvVars;
import hudson.model.EnvironmentContributor;

/**
* @author Kohsuke Kawaguchi
Expand Down Expand Up @@ -1232,6 +1234,25 @@ public void testSingleModuleEnvironmentVariables() throws Exception {
assertEquals(getActualRevision(p.getLastBuild(), "https://svn.jenkins-ci.org/trunk/hudson/test-projects/trivial-ant").toString(), builder.getEnvVars().get("SVN_REVISION"));
}

public void testRecursiveEnvironmentVariables() throws Exception {
EnvironmentContributor.all().add(new EnvironmentContributor() {
@Override public void buildEnvironmentFor(Run run, EnvVars ev, TaskListener tl) throws IOException, InterruptedException {
ev.put("TOOL", "ant");
ev.put("ROOT", "https://svn.jenkins-ci.org/trunk/hudson/test-projects/trivial-${TOOL}");
}
});
FreeStyleProject p = createFreeStyleProject("job-with-envs");
p.setScm(new SubversionSCM("$ROOT"));
CaptureEnvironmentBuilder builder = new CaptureEnvironmentBuilder();
p.getBuildersList().add(builder);
assertBuildStatusSuccess(p.scheduleBuild2(0));
assertTrue(p.getLastBuild().getWorkspace().child("build.xml").exists());
assertEquals("https://svn.jenkins-ci.org/trunk/hudson/test-projects/trivial-ant", builder.getEnvVars().get("SVN_URL"));
assertEquals(getActualRevision(p.getLastBuild(), "https://svn.jenkins-ci.org/trunk/hudson/test-projects/trivial-ant").toString(), builder.getEnvVars().get("SVN_REVISION"));
assertEquals("https://svn.jenkins-ci.org/trunk/hudson/test-projects/trivial-ant", builder.getEnvVars().get("SVN_URL_1"));
assertEquals(getActualRevision(p.getLastBuild(), "https://svn.jenkins-ci.org/trunk/hudson/test-projects/trivial-ant").toString(), builder.getEnvVars().get("SVN_REVISION_1"));
}

@Bug(1379)
public void testMultipleCredentialsPerRepo() throws Exception {
Proc p = runSvnServe(getClass().getResource("HUDSON-1379.zip"));
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/hudson/scm/SubversionSCMUnitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public void shouldSetEnvironmentVariablesWithSingleSvnModule() throws IOExceptio
SubversionSCM scm = mockSCMForBuildEnvVars();

ModuleLocation[] singleLocation = new ModuleLocation[] {new ModuleLocation("/remotepath", "")};
when(scm.getLocations(any(AbstractBuild.class))).thenReturn(singleLocation);
when(scm.getLocations(any(EnvVars.class), any(AbstractBuild.class))).thenReturn(singleLocation);

Map<String, Long> revisions = new HashMap<String, Long>();
revisions.put("/remotepath", 4711L);
Expand Down Expand Up @@ -83,7 +83,7 @@ public void shouldSetEnvironmentVariablesWithMultipleSvnModules() throws IOExcep
ModuleLocation[] locations = new ModuleLocation[] {
new ModuleLocation("/remotepath1", ""),
new ModuleLocation("/remotepath2", "")};
when(scm.getLocations(any(AbstractBuild.class))).thenReturn(locations);
when(scm.getLocations(any(EnvVars.class), any(AbstractBuild.class))).thenReturn(locations);

Map<String, Long> revisions = new HashMap<String, Long>();
revisions.put("/remotepath1", 4711L);
Expand Down