[PARTIALLY FIXED JENKINS-28632] Integration with Workflow to trigger a Workflow job #3
Conversation
|
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. |
| @@ -67,13 +73,13 @@ public void checkAndFire(DeploymentFacet facet) { | |||
| if (b!=null) { | |||
| // pass all the current parameters if we can | |||
| ParametersAction action = b.getAction(ParametersAction.class); | |||
| job.scheduleBuild(job.getQuietPeriod(), new UpstreamDeploymentCause(b), action); | |||
| parameterizedJobMixIn.scheduleBuild2(5, action); | |||
There was a problem hiding this comment.
🐜 Use parameterizedJobMixIn.scheduleBuild(action) directly (it already uses the proper quiet period)
There was a problem hiding this comment.
@amuniz That is incorrect as scheduleBuild is not expecting an Action but a Cause.
There was a problem hiding this comment.
Then use parameterizedJobMixIn.scheduleBuild2(parameterizedJobMixIn.getQuietPeriod(), new CauseAction(new UpstreamDeploymentCause(b)), action). Otherwise you are going to loose the UpstreamDeploymentCause (which I guess it's used somewhere later).
There was a problem hiding this comment.
I think you are still loosing some information here, since new UpstreamDeploymentCause(b) is not passed to scheduleBuild2.
There was a problem hiding this comment.
Right, but scheduleBuild2 doesn't allow you to add the Cause.
http://javadoc.jenkins-ci.org/jenkins/model/ParameterizedJobMixIn.html
There was a problem hiding this comment.
Did you try to wrap the cause in a CauseAction?
|
LGTM, but please fix the 🐜 |
|
looks like there are some regressions though.. |
|
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
|
@varmenise I have no clue why the two tests failing are related with this plugin.
|
| @@ -66,14 +73,13 @@ public void checkAndFire(DeploymentFacet facet) { | |||
| Run b = upstream.getBuildByNumber(n); | |||
| if (b!=null) { | |||
| // pass all the current parameters if we can | |||
| ParametersAction action = b.getAction(ParametersAction.class); | |||
| job.scheduleBuild(job.getQuietPeriod(), new UpstreamDeploymentCause(b), action); | |||
| parameterizedJobMixIn.scheduleBuild(new UpstreamDeploymentCause(b)); | |||
There was a problem hiding this comment.
Why are you ceasing to copy along the current ParametersAction?
You want to use scheduleBuild2, not scheduleBuild.
There was a problem hiding this comment.
@jglick Ouch, I thought just the opposite. Why is scheduleBuild2 better than scheduleBuild in this context?
|
Just fixed the issue with the failed tests. |
|
@reviewbybees done |
fbelzunc
changed the title
[PARTIALLY FIXED JENKINS-28632] Integration with Workflow to trigger a Workflow Job
This pull request partially integrates
deployment-notification-pluginwithWorkflowplugin.I will create a
awaitDeploymentstep on a future pull request. For this moment, I am only interested on the trigger integration.https://issues.jenkins-ci.org/browse/JENKINS-28632
@reviewbybees @jglick