Skip to content
This repository has been archived by the owner on Dec 15, 2021. It is now read-only.

[JENKINS-26093] Better support for build step parameters #69

Merged

Conversation

jglick
Copy link
Member

@jglick jglick commented Mar 3, 2015

Part of JENKINS-26093 so far.

@reviewbybees

@jglick jglick changed the title [WiP] [JENKINS-26093] Better support for build step parameters [JENKINS-26093] Better support for build step parameters Mar 3, 2015
@jglick
Copy link
Member Author

jglick commented Mar 3, 2015

@reviewbybees should be ready for review.

@stephenc
Copy link
Member

stephenc commented Mar 4, 2015

👍 from me but did you ever resolve the $class fight with @kohsuke

@tfennelly
Copy link
Member

👍

<script>
function loadParams() {
var div = $$('params');
new Ajax.Request('${descriptor.descriptorUrl}/parameters?job=' + encodeURIComponent($$('${jobFieldId}').value), {
Copy link
Member Author

Choose a reason for hiding this comment

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

Just occurred to me that this will not handle relative paths, need to fix that.

@jglick
Copy link
Member Author

jglick commented Mar 4, 2015

Build 953 from 495edb4 was stable (the PR builder is having trouble noting that here), so again @reviewbybees.

@stephenc
Copy link
Member

stephenc commented Mar 4, 2015

I'm still 👍 but I think we want @kohsuke to review the $class stuff as it seems against his vision

@tfennelly
Copy link
Member

👍

Note sure I totally understand it though (StaplerReferer etc`).

@jglick
Copy link
Member Author

jglick commented Mar 4, 2015

Regarding $class here: there is a ParameterValue.getValue which @kohsuke added recently (without any actual uses, AFAICT); JENKINS-26093 notes that an alternate approach under consideration was the creation of ParameterDefinition.createValue(Object), which would presumably let you specify the parameters as a Map<String,Object>. (At runtime the definitions would need to be looked up in the downstream project so you would know whether a String value was meant to become a StringParameterValue, PasswordParameterValue, etc.) If such an API were added and WF were to depend on the appropriate core version, this would still be an option, presumably with a new @DataBoundSetter taking such a map, and some more hacking around in Jelly and Snippetizer to make the Snippet Generator produce that.

Here (in 72860e9, the non-UI part of the PR) I am going with a more conservative change. parameters still accepts a list of actual ParameterValue objects, but the preferred mode is to use the $class data binding style, which (a) avoids the need to import Jenkins model classes, (b) does not trigger sandbox violations.

@jglick
Copy link
Member Author

jglick commented Mar 4, 2015

StaplerReferer is needed because StaplerRequest.findAncestor, used here to look up the WorkflowJob being configured so that a relative job path can be interpreted correctly, does not work when called from a dynamically-injected form fragment.

@jglick jglick merged commit 495edb4 into jenkinsci:master Mar 4, 2015
jglick added a commit that referenced this pull request Mar 4, 2015
Conflicts:
	CHANGES.md
@jglick jglick deleted the BuildTriggerStep.parameters-JENKINS-26093 branch March 4, 2015 16:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants