Allow Commit Notifications to be used as Workflow Build Steps.

[arthurschreiber]

[KostyaSha]

Right, that was i worked on, @jglick that what you missed for workflow support.

[arthurschreiber]

Yeah, so this allows the two commit notification build steps to be used in the workflow plugin. The changes are super simple, and a quick (if possible) release would make me very happy. 😄

[KostyaSha]

@arthurschreiber i would like wait @jglick review.

[lanwen]

@arthurschreiber can you write an example of wf script with this steps?

[arthurschreiber]

@lanwen Here's an example workflow script that I'm using together with these changes:

node {
  checkout scm

  step([$class: 'GitHubSetCommitStatusBuilder'])

  catchError {
    wrap([$class: 'AnsiColorBuildWrapper', 'colorMapName': 'XTerm']) {
      sh('script/cibuild')
    }
  }

  step([$class: 'GitHubCommitNotifier', resultOnFailure: 'FAILURE'])
}

[arthurschreiber]

I think there is something not working quite yet - and I believe this is because the GitHubCommitNotifier tries to read the status of the build, but the build is not finished yet when it is getting executed. I think refactoring GitHubSetCommitStatusBuilder and GitHubCommitNotifier might be necessary.

[arthurschreiber]

Okay, the following workflow script does the trick:

node {
  checkout scm

  step([$class: 'GitHubSetCommitStatusBuilder'])

  echo "${currentBuild.result}"

  try {
    wrap([$class: 'AnsiColorBuildWrapper', 'colorMapName': 'XTerm']) {
      sh('script/cibuild')
    }

    currentBuild.result = 'SUCCESS'
  } catch (err) {
    currentBuild.result = 'FAILURE'
  }

  step([$class: 'GitHubCommitNotifier', resultOnFailure: 'FAILURE'])
}

[KostyaSha]

So workflow doesn't set run results? Smells like a workflow bug.

[arthurschreiber]

@KostyaSha Not quite. In case the sh (or any previous step) fails and throws an error, no further steps are executed, so the final GitHubCommitNotifier is never executed.

When using a try/catch block (as in my example), the build status has to be set manually. This is documented in different help pages of workflow.

[KostyaSha]

That's unexpected way of workflow logic, step was performed on build but didn't set it's result (btw many plugins setting results directly). For non-standard cases i would expected save result state before execution and setting it back in case of skipping step execution.

[arthurschreiber]

From https://github.com/jenkinsci/workflow-plugin/blob/master/basic-steps/CORE-STEPS.md:

Running a notifier is trickier since normally a flow in progress has no status yet, unlike a freestyle project whose status is determined before the notifier is called. To help interoperate better with these, you can use the catchError step, or manually set a build status using currentBuild.result. See the help for the catchError step for examples.

[KostyaSha]

@arthurschreiber thanks for info.
Facepalm, catchError instead of providing sequential execution block.

[arthurschreiber]

@KostyaSha Maybe implementing a SimpleBuildWrapper instead of separate GitHubSetCommitStatusBuilder and GitHubCommitNotifier would be a better approach? The "pending" commit status could be set when starting the build, and the success or failure commit status could be sent from a Disposer after the build finished.

[KostyaSha]

BuildWrapper doesn't allow you specify the step/place when you want set status. And people may have cases when they don't want set pending status. I spent many time verifying different cases and having small steps imho the better variant that allows you create flexible flows.

[lanwen]

@arthurschreiber Can you rebase it? I'll going to merge this pr

[jenkinsadmin]

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

[arthurschreiber]

@lanwen I just rebased and pushed again. Please have a look.

[lanwen]

Thanks!

[jglick]

The PR seems fine so far as it goes, but yes in general it can be awkward to use publishers which depend on the current build’s result; catchError is available for compatibility with this sort of freestyle-oriented logic. If you implement a custom Step you can provide more flexible and intuitive behavior, at the cost of more code.

BTW

currentBuild.result = 'SUCCESS'

is a no-op.

[KostyaSha]

@jglick Writing workflow steps (or what class do you mean?) means killing other job types. Publishers and Builders will exist and handling results is step back to complex configurations and bad user experience.

[arthurschreiber]

@KostyaSha @lanwen Can you release a new version with the merged changes? ❤️

[arthurschreiber]

@jglick

BTW
currentBuild.result = 'SUCCESS'
is a no-op.

That is not really true, from what I can see. Even after executing a sh step, the build is marked as pending, unless the sh step failed. So if step([$class: 'GitHubCommitNotifier', resultOnFailure: 'FAILURE']) is reached, it will send a pending notification because the build result was never set to successful.

[lanwen]

@arthurschreiber sorry for the delay, was on NY vacation.

[lanwen]

released as 1.15.0

[arthurschreiber]

@lanwen Thank you!

[mikz]

I just tried this and must say it works! Nice job.

However, I was trying to use the new custom context feature (#100) and it does not work for me. It works for me with normal jobs, but not with workflow. Can someone confirm it works and it is just on my side?

[lanwen]

@mikz How did u try to use context feature with wf?

[mikz]

The same way as normal projects. Turning on the GitHub Project checkbox, filling the URL and in Advanced setting the Display name.

I have two builds, project-pr and project-build. The -pr is ghprb triggering the -build.
Both have set the custom display name. When I trigger -build manually it correctly sets the context and the link points to the -build. However, when -build is triggered by -pr, the context is ignored and the link points to the -pr. The -pr triggers -build by using Trigger/call builds on other projects build step.

[lanwen]

@mikz Please fill JIRA issue with github-plugin component. Also please attach config.xml of jobs. I'll take a look closer

[KostyaSha]

Didn't get what you are doing, but having standard builds and (especially) ghprb in one job or mixed job is bad idea. Firstly try to use clean job and if it doesn't work, then fill issue.

[mikz]

Unfortunately, Workflow job can't be used with ghprb. I'd love to have them in one. https://issues.jenkins-ci.org/browse/JENKINS-26591

[KostyaSha]

@mikz you can try https://github.com/jenkinsci/github-integration-plugin Workflow support in master, i will do one more beta release in evening, any testing would be appreciate, because i can't test all cases.