#284 emailing commiter on PRs from jenkins
Merged 5 years ago by lholecek. Opened 5 years ago by jmolet.
jmolet/greenwave improved-email  into  master

file modified
+60 -18
@@ -1,6 +1,7 @@ 

  /*

   * SPDX-License-Identifier: GPL-2.0+

   */

+ import groovy.json.*

  

  // 'global' var to store git info

  def scmVars
@@ -9,11 +10,44 @@ 

  

  node('master'){

      scmVars = checkout scm

+     scmVars.GIT_BRANCH_NAME = scmVars.GIT_BRANCH.split('/')[-1]  // origin/pr/1234 -> 1234

+ 

+     // setting build display name

+     def branch = scmVars.GIT_BRANCH_NAME

+     if ( branch == 'master' ) {

+         echo 'Building master'

+         currentBuild.displayName = 'master'

+     }

+     else if (branch ==~ /[0-9]+/) {

+         def pagureUrl = "https://pagure.io/greenwave/pull-request/${branch}"

+         def pagureLink = """<a href="${pagureUrl}">PR-${branch}</a>"""

+         try {

+             def response = httpRequest "https://pagure.io/api/0/greenwave/pull-request/${branch}"

+             // Note for future use: JsonSlurper() is not serialiazble (returns a LazyMap) and

+             // therefore we cannot save this back into the global scmVars. We could use

+             // JsonSlurperClassic() which returns a hash map, but would need to allow this in

+             // the jenkins script approval.

+             def content = new JsonSlurper().parseText(response.content)

+             pagureLink = """<a href="${pagureUrl}">${content.title}</a>"""

+         } catch (Exception e) {

+             echo 'Error using pagure API:'

+             echo e.message

+             // ignoring this...

+         }

+         echo "Building PR #${branch}: ${pagureUrl}"

+         currentBuild.displayName = "PR #${branch}"

+         currentBuild.description = pagureLink

+     }

  }

  

  timestamps {

  node('fedora-27') {

      checkout scm

+     scmVars.GIT_AUTHOR_EMAIL = sh (

+         script: 'git --no-pager show -s --format=\'%ae\'',

+         returnStdout: true

+     ).trim()

+ 

      sh '''

      sudo dnf -y builddep greenwave.spec

      sudo dnf -y install python3-flake8 python3-pylint python3-sphinx \
@@ -282,30 +316,38 @@ 

  } finally {

      // if result hasn't been set to failure by this point, its a success.

      def currentResult = currentBuild.result ?: 'SUCCESS'

+     def branch = scmVars.GIT_BRANCH_NAME

  

      // send pass/fail email

-     if (ownership.job.ownershipEnabled) {

-         def previousResult = currentBuild.previousBuild?.result

-         def SUBJECT = ''

-         def BODY = "${env.BUILD_URL}"

- 

-         if (previousResult == 'FAILURE' && currentResult == 'SUCCESS') {

-             SUBJECT = "Jenkins job ${env.JOB_NAME} #${env.BUILD_NUMBER} fixed."

-         }

-         else if (previousResult == 'SUCCESS' && currentResult == 'FAILURE' ) {

-             SUBJECT = "Jenkins job ${env.JOB_NAME} #${env.BUILD_NUMBER} failed."

+     def SUBJECT = ''

+     if ( branch ==~ /[0-9]+/) {

+         if (currentResult == 'FAILURE' ){

+             SUBJECT = "Jenkins job ${env.JOB_NAME}, PR #${branch} failed."

+         } else {

+             SUBJECT = "Jenkins job ${env.JOB_NAME}, PR #${branch} passed."

          }

+     } else if (currentResult == 'FAILURE') {

+         SUBJECT = "Jenkins job ${env.JOB_NAME} #${env.BUILD_NUMBER} failed."

+     }

  

-         if (SUBJECT != '') {

-             emailext to: ownership.job.primaryOwnerEmail,

-                      subject: SUBJECT,

-                      body: BODY

-         }

+     def RECIEPENT = scmVars.GIT_AUTHOR_EMAIL

+     if (ownership.job.ownershipEnabled && branch == 'master') {

+         RECIEPENT = ownership.job.primaryOwnerEmail

+     }

+ 

+     def BODY = "Build URL: ${env.BUILD_URL}"

+     if (branch ==~ /[0-9]+/){

+         BODY = BODY + "\nPull Request: https://pagure.io/greenwave/pull-request/${branch}"

+     }

+ 

+     if (SUBJECT != '') {

+         emailext to: RECIEPENT,

+                  subject: SUBJECT,

+                  body: BODY

      }

  

      // update Pagure PR status

-     def pagurePR = scmVars.GIT_BRANCH.split('/')[-1]  // origin/pr/1234 -> 1234

-     if (pagurePR ==~ /[0-9]+/) {  // PR's will only be numbers on pagure

+     if (branch ==~ /[0-9]+/) {  // PR's will only be numbers on pagure

          def resultPercent = (currentResult == 'SUCCESS') ? '100' : '0'

          def resultComment = (currentResult == 'SUCCESS') ? 'Build passed.' : 'Build failed.'

          def pagureRepo = new URL(scmVars.GIT_URL).getPath() - ~/^\// - ~/.git$/  // https://pagure.io/my-repo.git -> my-repo
@@ -315,7 +357,7 @@ 

              propagate: false,

              parameters: [

                  // [$class: 'StringParameterValue', name: 'PAGURE_REPO', value: 'https://pagure.io'],  // not needed if https://pagure.io

-                 [$class: 'StringParameterValue', name: 'PAGURE_PR', value: pagurePR],

+                 [$class: 'StringParameterValue', name: 'PAGURE_PR', value: branch],

                  [$class: 'StringParameterValue', name: 'PAGURE_REPO', value: pagureRepo],

                  [$class: 'StringParameterValue', name: 'PERCENT_PASSED', value: resultPercent],

                  [$class: 'StringParameterValue', name: 'COMMENT', value: resultComment],

Is this roughly what y'all were thinking?

Interesting side effect I've noticed since we've added the builds against the PRs:

with this logic:

    if (previousResult == 'FAILURE' && currentResult == 'SUCCESS') {
        SUBJECT = "Jenkins job ${env.JOB_NAME} #${env.BUILD_NUMBER} fixed."
    }
    else if (previousResult == 'SUCCESS' && currentResult == 'FAILURE' ) {
        SUBJECT = "Jenkins job ${env.JOB_NAME} #${env.BUILD_NUMBER} failed."
    }

This logic worked because we built against the same branch over and over. Now that we have PRs and master builds all intermingled, there isn't much useful in comparing previousResult vs currentResult. As it is now, for example, if build N against PR 123 fails, and build N+1 against PR 456 passes, the author of 456 gets a "build fixed" email.

Ideally to counter this we would need to be able to get the previousResult for a given branch, but I'm short of ideas of a good way do that in a low failure prone way.

Other option would be to rely on the new recipient logic:

    def RECIEPENT = scmVars.GIT_AUTHOR_EMAIL
    if (ownership.job.ownershipEnabled && branch == 'master') {
        RECIEPENT = ownership.job.primaryOwnerEmail
    }

and planning on most dev work happening outside of master... but I don't have a good feel for this since I'm not a main dev for the project. Open to suggestions of what to implement :)

Would it be possibly to set name for the build and retrieve it later? (The name could be just the remote branch name.)

Looks like it could be possible with code from here.

Wouldn't it be more correct to check who did submit the PR?

I would expect pagure to have an endpoint for telling that, although didn't check.

@lholecek has a good point here. This could be similar to what Travis CI does.

@lholecek I will try this...

@csomh Messing around with this: https://pagure.io/api/0/ I can obtain the username of the person who submitted the PR, but the API never returns the user's email address ( neither at GET /api/0/user/<username> or GET /api/0/<repo>/pull-request/<request id> ). It doesn't seem to even return my own email address using my personal token.

@jmolet the API not exposing email address might actually make sense...

1 new commit added

  • Setting display name
5 years ago

1 new commit added

  • now with even fancier build descriptions
5 years ago

@lholecek build name and descriptions are now set with this.

The question about the email notifications remains. My recommendation:
- Team list gets all failure notifications against the master branch
- Commit author gets all failure and success notifications against PR branches

Thoughts?

rebased onto 01d8d76fe69f65fae7fd80ab75e183eb67f6b996

5 years ago

Thanks @ralph , implemented that. Removing the [WIP] tag. Rebased onto master. If the tests pass this is good to merge from my perspective.

rebased onto d3f0626

5 years ago

+1

Having the link to PR is really nice bonus.

Though, the build for this PR failed for some reason (looks like it sends e-mail correctly to just the author now).

Started a rebuild to see if it was just a blip or not.

Rebuilding picked #285 to build, and of course it did... @jmolet please force push this PR. Thanks!

@csomh I don't have commit access to this repo :)

@jmolet I think @csomh meant git commit --amend --no-edit && git push --force so that Jenkins job runs again.

Anyway, I'm merging this since it shouldn't fail.

Pull-Request has been merged by lholecek

5 years ago