Skip to content

Expand environment variables in repository URL.#38

Merged
kutzi merged 11 commits intojenkinsci:masterfrom
jglick:doCheckRemote-masterEnvVars
Oct 7, 2013
Merged

Expand environment variables in repository URL.#38
kutzi merged 11 commits intojenkinsci:masterfrom
jglick:doCheckRemote-masterEnvVars

Conversation

@jglick
Copy link
Copy Markdown
Member

@jglick jglick commented Mar 1, 2013

Expand environment variables in repository URL in job confguration during validation check, affecting also entering of credentials.

This should allow you to use the same $JENKINS_HOME contents while switching the physical location of your Subversion server using a startup environment.

(Has no effect when Validate repository URLs up to the first variable name is checked.)

Also expands build variables (e.g. those defined in a containing folder property) when defining SVN_* variables, which would otherwise go unset.

Might solve JENKINS-11667.

…ring validation check, affecting also entering of credentials.

This should allow you to use the same $JENKINS_HOME contents while switching the physical location of your Subversion server using a startup environment.
(Has no effect when "Validate repository URLs up to the first variable name" is checked.)
@buildhive
Copy link
Copy Markdown

Jenkins » subversion-plugin #116 SUCCESS
This pull request looks good
(what's this?)

@buildhive
Copy link
Copy Markdown

Jenkins » subversion-plugin #117 UNSTABLE
Looks like there's a problem with this pull request
(what's this?)

@buildhive
Copy link
Copy Markdown

Jenkins » subversion-plugin #118 UNSTABLE
Looks like there's a problem with this pull request
(what's this?)

@buildhive
Copy link
Copy Markdown

Jenkins » subversion-plugin #131 UNSTABLE
Looks like there's a problem with this pull request
(what's this?)

@jglick
Copy link
Copy Markdown
Member Author

jglick commented May 24, 2013

I am not sure what the PerJobCredentialStoreTest.testRemoteBuild failure indicates; all tests pass for me locally. The earlier failure of SubversionSCMTest.testMultipleCredentialsPerRepo suggests to me that the tests are simply unreliable.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Actually this is wrong, because getLocations ignores build when env is set. Need to consider both combined.

@buildhive
Copy link
Copy Markdown

Jenkins » subversion-plugin #138 UNSTABLE
Looks like there's a problem with this pull request
(what's this?)

…ild).

A user reported that $SVN_REVISION_1 etc. were not getting set otherwise.
@buildhive
Copy link
Copy Markdown

Jenkins » subversion-plugin #139 SUCCESS
This pull request looks good
(what's this?)

@jglick
Copy link
Copy Markdown
Member Author

jglick commented Jun 6, 2013

@kutzi are you reviewing PRs in this plugin, or should I ask @ndeloof etc.? I am not sure I follow all the logic surrounding variable expansion, so advice from people more familiar with the plugin would be welcome.

@kutzi
Copy link
Copy Markdown
Member

kutzi commented Jun 8, 2013

Sometimes I review PRs here, but there are several areas which I'm not familiar with, either, and variable expansion is definitely one of them.

@buildhive
Copy link
Copy Markdown

Jenkins » subversion-plugin #143 SUCCESS
This pull request looks good
(what's this?)

@buildhive
Copy link
Copy Markdown

Jenkins » subversion-plugin #144 FAILURE
Looks like there's a problem with this pull request
(what's this?)

@jenkinsadmin
Copy link
Copy Markdown
Member

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@buildhive
Copy link
Copy Markdown

Jenkins » subversion-plugin #155 UNSTABLE
Looks like there's a problem with this pull request
(what's this?)

@jglick
Copy link
Copy Markdown
Member Author

jglick commented Jul 22, 2013

Reporter of original issue seems satisfied with the current state of this branch.

Copy link
Copy Markdown
Member

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

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

Choose a reason for hiding this comment

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

Okay, makes sense

@kutzi
Copy link
Copy Markdown
Member

kutzi commented Oct 7, 2013

As far as I'm able to understand this, looks fine.
As no one else seems to like to review this and it doesn't seem to break anything, I'm going to merge it

kutzi added a commit that referenced this pull request Oct 7, 2013
Expand environment variables in repository URL.
@kutzi kutzi merged commit 0799ee6 into jenkinsci:master Oct 7, 2013
@jglick jglick deleted the doCheckRemote-masterEnvVars branch October 7, 2013 17:47
@jglick
Copy link
Copy Markdown
Member Author

jglick commented Oct 15, 2013

BTW any plans for a trunk release? (I know the refactoring branch was said to still need some love.)

@kutzi
Copy link
Copy Markdown
Member

kutzi commented Oct 15, 2013

Done. After I had to skip a version number and lost another hour of my life because of http://jira.codehaus.org/browse/MRELEASE-812 :(

@jglick
Copy link
Copy Markdown
Member Author

jglick commented Oct 15, 2013

Just be thankful your $LOCALE is not tr_TR! (Hint: String.toUpper/LowerCase, ı, İ, all hell breaks loose)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants