Skip to content
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

Jump to bottom

[FIX-JENKINS-41421] Extend StepImpl to pass along submitterParameter #820

Merged
merged 1 commit into from Feb 14, 2017
Merged

[FIX-JENKINS-41421] Extend StepImpl to pass along submitterParameter #820

merged 1 commit into from Feb 14, 2017

Conversation

scherler
Copy link
Collaborator

Description

See JENKINS-41421.

Basic change in PipelineStepImpl.java rest is to adopt to the v. 2.5 of pipeline-input-step

Submitter checklist

  • Link to JIRA ticket in description, if appropriate.
  • Change is code complete and matches issue description
  • Appropriate unit or acceptance tests or explanation to why this change has no tests
  • Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.
  • Ran Acceptance Test Harness against PR changes.

Reviewer checklist

  • Run the changes and verified the change matches the issue description
  • Reviewed the code
  • Verified that the appropriate tests have been written or valid explanation given

@reviewbybees

@scherler
Copy link
Collaborator Author

@i386 I tried to add the submitterParameter to the inputParameter step as shown in scherler/jenkins-testfiles@8745249 but that is no good. Can you provide an example to pass submitterParameter to the inputParameter?

jglick
jglick reviewed Feb 13, 2017
View reviewed changes
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

It is alarming that you are copying all this code from InputStepExecution. Probably this should all be rewritten.

@michaelneale
Copy link
Member

@jglick where is it copied from? What should be all rewritten?

@michaelneale
Copy link
Member

@scherler the ticket linked talks about text areas - which I don't think this addresses. I am confused as to what this is about.

@jglick
Copy link
Member

jglick commented Feb 13, 2017

where is it copied from?

InputStepExecution

@scherler
Copy link
Collaborator Author

@jglick yeah, since we have our own implementation of it. Maybe @vivek has some thoughts.

@michaelneale the ticket talks about that the submitterParameter is not passed to the flow and in case you only have one parameter that leads to passsing a string not a map to the pipeline api.

@michaelneale
Copy link
Member

@scherler but not about Text input (text area?) which seemed one of the other subjects?

@vivek will be reviewing this, and this may be related to: https://issues.jenkins-ci.org/browse/JENKINS-41044 which he also wanted to look at, so lets see what he thinks.

@scherler
Copy link
Collaborator Author

@michaelneale textarea had been supported by the first implementation. The problem is that without the PR the linked pipeline cannot be submitted in BO without causing an error.

vivek
vivek reviewed Feb 14, 2017
View reviewed changes
...ipeline-api-impl/src/main/java/io/jenkins/blueocean/rest/impl/pipeline/PipelineStepImpl.java
@@ -241,6 +243,12 @@ private Object parseValue(InputStepExecution execution, JSONArray parameters, St
}
mapResult.put(name, convert(name, v));
}
// If a destination value is specified, push the submitter to it.
String valueName = input.getSubmitterParameter();
if (valueName != null && !valueName.isEmpty()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps use StringUtils.isNotBlank(valueName)?

Copy link
Collaborator Author

@scherler scherler Feb 14, 2017 •
edited

Choose a reason for hiding this comment

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

@vivek as @jglick pointed out, this comes from https://github.com/jenkinsci/pipeline-input-step-plugin/blob/1031ad8008f2b84e1ca331e3eff7839d97922169/src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputStepExecution.java how could we reduce that redundancy?
...but yeah good point!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, possibly InputStepImpl.parseValue() should be refactored to avoid working off form submission and work just off JSONArray and also expose this and some other method public. I noted that time in PR/comment but missed to follow it up with JIRA/PR in pipeline-onput-step plugin. I will follow it up.

@scherler
Copy link
Collaborator Author

@michaelneale BTW this PR fixes https://issues.jenkins-ci.org/browse/JENKINS-41044

@vivek
Copy link
Collaborator

vivek commented Feb 14, 2017 •
edited

@scherler LGTM 🐝 pending ATH run and a test case to cover submitter parameter:) Thanks for fixing:)

@michaelneale
Copy link
Member

@scherler NICE!

Checked it too, looks good. ATH is happy - and @vivek gave it blessing. Nice catch!

And yes ideally wouldn't be repeating code in InputStepExecution but that was existent before this PR.

🐝

@scherler scherler merged commit 622aff8 into master Feb 14, 2017
@scherler scherler deleted the JENKINS-41421_input_steps_parameter_submitterParameter_not_supported branch February 14, 2017 11:04
@scherler scherler restored the JENKINS-41421_input_steps_parameter_submitterParameter_not_supported branch February 14, 2017 11:04
@scherler scherler deleted the JENKINS-41421_input_steps_parameter_submitterParameter_not_supported branch February 14, 2017 11:04
i386 pushed a commit that referenced this pull request Feb 15, 2017
@scherler
[master] Extend StepImpl to pass along submitterParameter (#820)
d849892
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vivek vivek vivek left review comments

@jglick jglick jglick left review comments

Assignees
No one assigned
Labels
needs-review ready-for-merge
Projects
None yet
Milestone
No milestone
4 participants
@scherler @michaelneale @jglick @vivek