Skip to content

Fixing build parameters not being expanded in slave client name format#52

Merged
rpetti merged 1 commit intojenkinsci:masterfrom
stuartrowe:master
Jul 17, 2014
Merged

Fixing build parameters not being expanded in slave client name format#52
rpetti merged 1 commit intojenkinsci:masterfrom
stuartrowe:master

Conversation

@stuartrowe
Copy link
Contributor

swapping order of param substitution when getting effective client name so build parameters put in the slave client name format are substituted.

…me so build parameters are properly evaluated
@cloudbees-pull-request-builder

plugins » perforce-plugin #102 SUCCESS
This pull request looks good

@jenkinsadmin
Copy link
Member

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

@oleg-nenashev
Copy link
Member

Exposure of build parameters to client name is hazardous, because Perforce Plugin won't be able to handle variables during the polling, workspace cleanup, etc.

Would you like to create a JIRA issue for the case and then submit the PR to https://github.com/jenkinsci/perforce-plugin/compare/variables_handling_refactoring ? I'll merge your change together with the updated variables resolver

@rpetti
Copy link
Member

rpetti commented May 23, 2014

Exposure of build parameters to the client name is already being done and is a widely used feature. During polling, the parameter default is used. This pull request simple allows parameters in the 'client name format for slaves' option to also be resolved, which was admittedly an oversight on my part.

@oleg-nenashev
Copy link
Member

I agree that the PR should be somehow merged. I've just mentioned possible (and well-known) downsides.

@oleg-nenashev oleg-nenashev self-assigned this May 23, 2014
@oleg-nenashev oleg-nenashev added this to the in-progress milestone May 23, 2014
@oleg-nenashev
Copy link
Member

Merged the PR into #54

@rpetti rpetti merged commit 1178ed2 into jenkinsci:master Jul 17, 2014
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