basic Jenkins pipeline support for notifier #185

balihb · 2016-10-27T13:42:12Z

This is a very rough Jenkins Pipeline support with code duplication that should somehow be refactored.
An example of such pipeline is like this:
Jenkinsfile.txt

coveralls · 2016-10-27T14:22:59Z

Coverage decreased (-0.1%) to 90.962% when pulling 6170fa8 on balihb:master2 into 7fe94de on uber:master.

ascandella · 2016-10-27T18:44:13Z

Very cool! Is there some way for me to test this? I've never used jenkinsfile/pipeline before

balihb · 2016-10-28T10:04:30Z

there is a repository here with the proper Jenkinsfile:
https://github.com/balihb/boot-react/blob/master/Jenkinsfile
The check step is currently failing, so I can test build failure. I'm currently using staging area for code review. I haven't modified the Phabricator build wrapper to work with Jenkins Pipeline.
I have the following build plan step in Harbormaster (make http request):
https://myci/buildByToken/buildWithParameters?job=${repository.callsign}_review&DIFF_ID=${buildable.diff}&PHID=${target.phid}&token=mytoken&STREF=${repository.staging.ref}
The job is configured as a pipeline job, with the post parameters as build parameters and the following SCM config:

ascandella · 2016-10-28T16:52:11Z

Ok, so it sounds like this is still a Work-In-Progress commit then, I'll add a WIP label and then ping me when this is ready for review.

aadis · 2017-01-19T13:15:59Z

We'd be interested in this as well.

balihb · 2017-01-24T00:01:03Z

I'll update the PR as soon as I can.
Some notes:

no wrapper support, just notification (might be possible, never tried)
no coverage support since cobertura has no pipeline support. for now and the type hierarchy stops the plugin to support coverage (run vs abstractbuild. let me know if anyone knows a solution) (also refer to: JENKINS-30700 / #50 Jenkins pipeline support jenkinsci/cobertura-plugin#55 )
introducing pipeline tests is a whole bunch of big work in itself, so it might take a while wile I can produce more than just some self tested code

aadis · 2017-01-24T15:22:12Z

As you mentioned the cobertura plugin is right now seeing active work on pipeline support. The PR#55 builds and works.

It might be a good idea to test against that (or I can do the testing & report back if you update this PR)

CLAassistant · 2017-01-31T15:27:22Z

All committers have signed the CLA.

coveralls · 2017-01-31T15:35:46Z

Coverage decreased (-0.2%) to 91.139% when pulling 28dacfb on balihb:master2 into 65de8c0 on uber:master.

balihb · 2017-01-31T15:37:49Z

The solution is getting more and more hacky. I can't exactly base a PR on an unreleased dependency.
So still only notification support.

coveralls · 2017-01-31T15:42:08Z

Coverage decreased (-0.2%) to 91.139% when pulling 28dacfb on balihb:master2 into 65de8c0 on uber:master.

coveralls · 2017-01-31T15:46:31Z

Coverage decreased (-0.2%) to 91.139% when pulling 28dacfb on balihb:master2 into 65de8c0 on uber:master.

balihb · 2017-02-01T09:57:38Z

@sectioneight if I create proper pipeline tests would this PR be eligible for merging into master? There is still no coverage and build wrapper support, but it would be great have the basic pipeline support in the released plugin.

ascandella · 2017-02-01T17:25:01Z

Absolutely, if there's unit test coverage, at least a little bit of documentation (maybe in the advanced section) and the code is reasonable then this is purely additive. No need to implement everything all at once, in fact we prefer smaller PRs. Also feel free to open up a Work In Progress PR if you want to get a sanity check / feedback on the general approach.

…

On Wed, Feb 1, 2017 at 1:57 AM balihb ***@***.***> wrote: @sectioneight <https://github.com/sectioneight> if I create proper pipeline tests would this PR be eligible for merging into master? There is still no coverage and build wrapper support, but it would be great have the basic pipeline support in the released plugin. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#185 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAF7P147HU8ZcRIDKgsuPoHY9lbUSG9Cks5rYFcUgaJpZM4KiXMt> .

ascandella · 2017-02-01T17:26:14Z

Oh, i see, we already have a PR :) I was assuming this was an issue while responding via email.

Would you like us to take a look at this now or are there changes forthcoming?

hanjunx · 2017-02-02T00:32:51Z

lgtm Hanjun

…

On Thu, Feb 2, 2017 at 1:26 AM, Aiden Scandella ***@***.***> wrote: Oh, i see, we already have a PR :) I was assuming this was an issue while responding via email. Would you like us to take a look at this now or are there changes forthcoming? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#185 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACW3ywqQVYwuaPSSWLgFMFjuifI3BL2Zks5rYMBFgaJpZM4KiXMt> .

cmmata · 2017-05-23T10:36:23Z

Is this PR still valid?

balihb · 2017-05-29T12:38:29Z

it is valid. I just haven't had the time to write the tests for it.

aadis · 2017-07-11T13:24:59Z

jenkinsci/cobertura-plugin#55 has been merged, so it'd be great if this PR can make sure cobertura support is enabled.

We're using this PR directly, and other than the coverage reporting/comparison, it's been working great!

julienmathevet · 2017-09-13T12:04:03Z

+1

balihb · 2017-10-31T16:25:17Z

I finally had time to finish this up. Please try it out!
I want to follow it up with a few more PR like wrapper support and pipeline dsl support among others.

coveralls · 2017-10-31T16:28:48Z

Coverage decreased (-0.1%) to 90.619% when pulling 9a8b6a3 on balihb:master2 into 99a4231 on uber:master.

balihb · 2017-11-02T17:19:38Z

@sectioneight ping :D

ascandella · 2017-11-06T20:16:30Z

@balihb I'm not working on this anymore, perhaps @jjx could take a look or knows somebody who's current?

balihb · 2017-11-14T16:27:19Z

maybe @artms as the latest commiter?

sascha-egerer · 2018-05-09T13:16:03Z

Just did a test and works fine here. This is my Jenkinsfile

pipeline {
  agent any
  parameters {
    string(name: 'PHID', defaultValue: '', description: '')
    string(name: 'DIFF_ID', defaultValue: '', description: '')
  }
  stages {
    stage('build') {
      steps {
        sh 'echo foo'
      }
    }
  }
  post {
    always {
        script {
            // workaround. Seems to be a bug!?
            currentBuild.result = currentBuild.currentResult
        }
        step([$class: 'PhabricatorNotifier', commentOnSuccess: true, commentWithConsoleLinkOnFailure: true])
    }
  }
}

youngmit · 2018-05-14T18:13:10Z

Where does this stand? We would also really appreciate this PR getting merged. It has a "work in progress" tag; is this still WIP? Perhaps removing that tag would help to get the attention of the maintainers, as it appears to be ready for review. @sectioneight seemed initially enthusiastic about this PR, but is apparently no longer involved in maintaining the project. Who are the current maintainers?

balihb · 2018-05-14T18:32:28Z

based on the commit history noone. we are thinking about proposing to maintain it in the future.

…

On Mon, May 14, 2018, 20:15 Mitchell T.H. Young ***@***.***> wrote: Where does this stand? We would also really appreciate this PR getting merged. It has a "work in progress" tag; is this still WIP? Perhaps removing that tag would help to get the attention of the maintainers, as it appears to be ready for review. @sectioneight <https://github.com/sectioneight> seemed initially enthusiastic about this PR, but is apparently no longer involved in maintaining the project. Who are the current maintainers? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#185 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACvMmM8Y3Imjvyt5dCDSXgtiDy8yQrADks5tycmogaJpZM4KiXMt> .

kageiit · 2018-05-14T20:04:44Z

Im ok merging this if its been staged and working without breaking backwards compatibility

kageiit · 2018-05-14T20:05:33Z

@balihb please sign the CLA before we can merge this

kageiit · 2018-05-14T20:07:46Z

@balihb Can you please also add some documentation to the advanced section

balihb · 2018-05-15T15:18:04Z

I've added a fix for the null pointer exception in the pipeline.
Docs is in progress.

sascha-egerer · 2018-05-15T15:24:17Z

@balihb could you please take a look at my Jenkinsfile i've posted above? Is it possible to fix that issue the workaround is required for? currentBuild.result is empty and is copied from currentBuild.currentResult in my Jenkinsfile.

balihb · 2018-05-15T15:29:39Z

it should be fixed by my latest change.

kageiit · 2018-05-15T16:16:42Z

build.gradle


  optionalJenkinsPlugins 'org.jenkins-ci.plugins:junit:1.6@jar'
-  optionalJenkinsPlugins 'org.jenkins-ci.plugins:cobertura:1.9.6@jar'
+  optionalJenkinsPlugins 'org.jenkins-ci.plugins:cobertura:1.11@jar'


Why is this required? Can we decouple this change?

Answered in #185 (comment)

sounds good

balihb · 2018-05-15T16:21:24Z

this is required, since, this is the first cobertura plugin version, that supports pipeline.

…

On Tue, May 15, 2018, 18:18 Gautam Korlam ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In build.gradle <#185 (comment)> : > @@ -47,7 +47,7 @@ dependencies { jenkinsPlugins ***@***.***' optionalJenkinsPlugins ***@***.***' - optionalJenkinsPlugins ***@***.***' + optionalJenkinsPlugins ***@***.***' Why is this required? Can we decouple this change? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#185 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACvMmCfbmlR_wI-h7X3L7KcXfEoyxhPWks5tyv_TgaJpZM4KiXMt> .

balihb · 2018-05-16T13:44:17Z

documentation added. Hope it is ok. let me know if I should change anything about it.

kageiit · 2018-05-16T14:28:37Z

src/main/java/com/uber/jenkins/phabricator/BuildResultProcessor.java


    public Result getBuildResult() {
-        return this.build.getResult();
+        if (this.build.getResult() == null) {


Can you explain why we would want to return success on null? It seems a bit counterintuitive. Are there any api docs defining this new behavior?

on oldschool freestyle jobs after the build reached the last build step it set the build result to success, but in pipeline there is no definite end of build blocks, so it won't explicitly set SUCCESS, hence the previous workaround, where it was needed to run this before the notifier:

currentBuild.result = currentBuild.result ?: 'SUCCESS'

The phab notifier build step need not necessarily be the last step. If someone uses pipeline and not this plugin, they would have to workaround this anyways, so this logic should not be in this plugin. This should be reported upstream to jenkins.

null == success so far. IMHO it was a genuine missing null check in the plugin code. In freestyle it is always set and in pipeline it is always considered SUCCESS.

makes sense

kageiit · 2018-05-16T14:31:24Z

src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java

        resultProcessor.sendComment(commentWithConsoleLinkOnFailure);

-        return true;
+        return;


can remove this redundant return at the end of a void method

kageiit · 2018-05-16T14:32:10Z

src/main/java/com/uber/jenkins/phabricator/coverage/CoberturaCoverageProvider.java


        // Fallback to scanning for the reports
-        copyCoverageToJenkinsMaster(build);
+        copyCoverageToJenkinsMaster(build, workspace);


just inline build.getWorkspace() here and avoid the local workspace variable

in the new Jenkins API you can't use Build, but must use Run, but the run interface won't provide a getWorkspace method. I can change this call to this:

copyCoverageToJenkinsMaster(build, getWorkspace());

but this is the best I can do.

Lets leave it as is then

kageiit · 2018-05-16T16:47:01Z

src/main/java/com/uber/jenkins/phabricator/BuildResultProcessor.java


    public Result getBuildResult() {
-        return this.build.getResult();
+        if (this.build.getResult() == null) {


makes sense

khawaga · 2018-05-16T22:24:14Z

Any ETA on when this will be available (plugin version update)?

arcadien · 2018-06-12T10:38:36Z

Up? everyting seems ready? Please at least allow us to download the plugin from Travis integration server, thanks

kageiit · 2018-06-12T14:05:31Z

You can use ./gradlew jpi to build the plugin locally till a release is cut

arcadien · 2018-06-12T14:44:29Z

Hi, just done. Thanks Le mar. 12 juin 2018 à 16:07, Gautam Korlam <notifications@github.com> a écrit :

…

You can use ./gradlew jpi to build the plugin locally till a release is cut — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#185 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AByN11jNKE7jT6DjIyYNqK3fYkqt2okWks5t78skgaJpZM4KiXMt> .

bhush9 · 2018-06-30T12:54:01Z

Hi,

We in KDE would very much like to use this on our build servers, is there any chance to get it released under plugin upgrades as we'd rather use it from released version instead of unstable git builds.

Thanks.

ascandella added the work-in-progress label Oct 28, 2016

balihb force-pushed the master2 branch 2 times, most recently from d4ac341 to 28dacfb Compare January 31, 2017 15:32

balihb force-pushed the master2 branch from 28dacfb to 9aa1368 Compare October 31, 2017 16:20

balihb changed the title ~~basic Jenkins pipeline support for notifier (removed code duplication)~~ basic Jenkins pipeline support for notifier Oct 31, 2017

kageiit approved these changes May 14, 2018

View reviewed changes

kageiit removed the work-in-progress label May 14, 2018

balihb force-pushed the master2 branch from 026cc4c to 02f058b Compare May 15, 2018 15:16

balihb force-pushed the master2 branch from 02f058b to b8967e1 Compare May 15, 2018 15:47

kageiit reviewed May 15, 2018

View reviewed changes

balihb force-pushed the master2 branch from b8967e1 to 6ced745 Compare May 16, 2018 13:43

kageiit suggested changes May 16, 2018

View reviewed changes

balihb force-pushed the master2 branch from 6ced745 to 9b1fd2c Compare May 16, 2018 14:52

Jenkins pipeline support for notifier

9a8b6a3

balihb force-pushed the master2 branch from 9b1fd2c to 9a8b6a3 Compare May 16, 2018 16:31

kageiit approved these changes May 16, 2018

View reviewed changes

kageiit merged commit 93043b1 into uber-archive:master May 16, 2018

AttwellBrian mentioned this pull request Feb 20, 2019

NPE in PhabricatorNotifier, when used with pipeline (still) #317

Closed

OCram85 mentioned this pull request Jun 5, 2020

[Question] Is it possible to add custom phabricator comment from a jenkins pipeline step? #343

Closed

basic Jenkins pipeline support for notifier #185

basic Jenkins pipeline support for notifier #185

Uh oh!

Conversation

balihb commented Oct 27, 2016

Uh oh!

coveralls commented Oct 27, 2016

Uh oh!

ascandella commented Oct 27, 2016

Uh oh!

balihb commented Oct 28, 2016

Uh oh!

ascandella commented Oct 28, 2016

Uh oh!

aadis commented Jan 19, 2017

Uh oh!

balihb commented Jan 24, 2017

Uh oh!

aadis commented Jan 24, 2017

Uh oh!

CLAassistant commented Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jan 31, 2017

Uh oh!

balihb commented Jan 31, 2017

Uh oh!

coveralls commented Jan 31, 2017

Uh oh!

coveralls commented Jan 31, 2017

Uh oh!

balihb commented Feb 1, 2017

Uh oh!

ascandella commented Feb 1, 2017 via email

Uh oh!

ascandella commented Feb 1, 2017

Uh oh!

hanjunx commented Feb 2, 2017 via email

Uh oh!

cmmata commented May 23, 2017

Uh oh!

balihb commented May 29, 2017

Uh oh!

aadis commented Jul 11, 2017

Uh oh!

julienmathevet commented Sep 13, 2017

Uh oh!

balihb commented Oct 31, 2017

Uh oh!

coveralls commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

balihb commented Nov 2, 2017

Uh oh!

ascandella commented Nov 6, 2017

Uh oh!

balihb commented Nov 14, 2017

Uh oh!

sascha-egerer commented May 9, 2018

Uh oh!

youngmit commented May 14, 2018

Uh oh!

balihb commented May 14, 2018 via email

Uh oh!

kageiit commented May 14, 2018

Uh oh!

kageiit commented May 14, 2018

Uh oh!

kageiit commented May 14, 2018

Uh oh!

balihb commented May 15, 2018

Uh oh!

sascha-egerer commented May 15, 2018

Uh oh!

balihb commented May 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CLAassistant commented Jan 31, 2017 •

edited

Loading

coveralls commented Oct 31, 2017 •

edited

Loading