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

[FIXED JENKINS-38850] - Pass params to ParametersAction to fix shouldSchedule() #115

Merged
merged 3 commits into from
Jun 13, 2017
Merged

[FIXED JENKINS-38850] - Pass params to ParametersAction to fix shouldSchedule() #115

merged 3 commits into from
Jun 13, 2017

Conversation

oleg-nenashev
Copy link
Member

Just a quick bugfix with a set of direct unit tests for the related functionality. The issue itself comes from the partially correct fix of JENKINS-36124 in #100, which was a SECURITY-170 fix attempt.

Functional tests are available in #106

https://issues.jenkins-ci.org/browse/JENKINS-38850

@reviewbybees @cohencil @t0r0X @aheritier

jglick
jglick reviewed May 24, 2017
View reviewed changes
src/test/java/com/tikal/jenkins/plugins/multijob/test/MultiJobParametersActionTest.java Outdated
public class MultiJobParametersActionTest {

@Test
public void ShouldMergeEmptyParameters() {
Copy link
Member

Choose a reason for hiding this comment

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

Why the C#-style casing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, was hacking C# yesterday and pressed shift automatically. Will fix

aheritier
aheritier approved these changes May 24, 2017
View reviewed changes
Copy link
Member

@aheritier aheritier left a comment

Choose a reason for hiding this comment

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

LGTM 🐝
Thanks a lot @oleg-nenashev

@oleg-nenashev
Copy link
Member Author

My fix is probably incomplete for parameters loaded from the disk.
but it should not impact the queue logic anyway.... at least until MultiJob supports Pipeline.
Will fix it anyway.

@ghost
Copy link

ghost commented May 24, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@oleg-nenashev
Copy link
Member Author

@reviewbybees done
@hagzag PTAL, it seems to be a serious regression though there is not so much votes in JIRA

@oleg-nenashev
Copy link
Member Author

@hagzag gentle ping

@hagzag
Copy link
Member

hagzag commented Jun 12, 2017

@oleg-nenashev @cohencil is already on this PR, we were planning on releasing last week, I hope we get to it this week. I will keep you posted (or you will get notified via this thread). Sorry about the inconvenience.

@cohencil cohencil merged commit 3d22acf into jenkinsci:master Jun 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jglick jglick jglick left review comments

@aheritier aheritier aheritier approved these changes

Assignees
No one assigned
Labels
in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@oleg-nenashev @hagzag @jglick @aheritier @cohencil