Thanks for the feedback on this, André! This project's being developed by one of our UCOSP students this semester. He's quickly getting up to speed on not just this project but on Review Board's code and various use cases as well. I'm sure he'll benefit greatly from your feedback and from how you're making use of Review Board are your company.
James, André's a user of Review Board who has provided us many patches to enhance Review Bot and other parts of the project. His feedback will hopefully be useful to you!

Thanks! I'm happy to be a beta tester for this feature. 

I will try it later. I found some things for improvements. But I will wait a little bit until the major parts are added... :-)

By the way... I saw 'git only at the moment' - just ask me if you need help for Mercurial.

Great! Thanks for helping test this! I'll let you know once it's in a more usable state, and then give you some instructions for configuring it. I'm not certain that the 'git only' restriction still applies, I'll have to do some testing with other SCMs first.

Hi André, I feel like this latest revision is decent enough shape to be tested. Let me know if you still are wanting to try it out, I can write up some instructions for you on how to set it up.

Hi James, yeah, thanks! Little introduction would be nice. :-)

Hi Andre, sorry for taking long to get back to you. Here are the setup instructions: https://www.notion.so/reviewboard/Setup-Instructions-3c1a98414eb743c980b7729301bccbc3

Hi James,
thanks for the info. I tested it today. First of all... it works! :-)

I found some points that will be a little bit annoying if I roll it out as a replacement to current jenkins bot.

Any jenkins job needs to configure RB token and URL. And it is always the same. That is a pain to add it every time. Also I don't want to give any developer an API token for an ADMIN account. ;-) It would be configurable once per jenkins instance like VMWare's Reviewbot did. And every jenkins-job developer can just put the build step without any further configuration. And without giving ADMIN access.
The API token should be stripped down to a non-admin-token. I don't want to give a full-admin-token to someone else. Is it possible to create a special user with special rights (manually in database?)?
Is it possible to let review board poll the triggered jenkins job? I think jenkins will return an URL of current triggered build. So review board could check the state of that job without adding an explicit "publish state to reviewboard" post-build-step.
Custom message for post-build-step. Current master branch of VMWare's "publish result" allows to add a custom message. Of course the new status API is not an usual "review comment". But is it possible to add a message for that build? We uses a single multi-job build to trigger a lot more builds for different systems. After all builds are finished the multi-job posts a custom message with links to that builds. See https://screenshots.firefox.com/pgv4BwhL998f1loM/rb.governikus.de as an example. So a tester could just lick that link and download an APK for Android an manually check everything is fine. Everything a computed by our scripts. We only need a simple message box for text and markdown. Is it possible to add that as bottom_text or something like this?
Pipeline-Syntax: We forced jenkins developers to use the new declarative pipeline syntax for jenkins job instead of "freestyle" ones. https://jenkins.io/blog/2017/02/03/declarative-pipeline-ga/ ... is it possible to add support for a pipeline syntax?


Thanks for all!

Oh, yeah.... is it possible re-trigger that build from somethere? Maybe manually from jenkins and jenkins uses the same STATUS_ID to update the state? Because sometimes a build is a little bit flaky if some servers are down. So the jenkins will fail sometimes ... after someone restarts the build it will be finished.

by the way... there are some python libs for jenkins.
* https://jenkins-webapi.readthedocs.io/en/latest/
* https://pypi.python.org/pypi/jenkinsapi
* https://pypi.python.org/pypi/python-jenkins/0.4.15
They support to get result of a build job, too. :-)

Hey André,
Thanks for the great feedback! And thanks for the link to that jenkins Python library.

I agree, I'll look into another solution for the API token. The other RB integrations send this token as a parameter, but build parameters in Jenkins aren't private. I'll look into how reviewbot is doing this and take some inspiration.
The API token should technically for the "jenkinsci" user (which manages the status update), the admin's works as well though. I'll try to expose this in the integration pane if possible.
I discussed polling with Christian, currently he doesn't want polling in RB as that's a larger task. This could be a potential future change.
Yup, comments should be possible.
I'll have to look into this further, I don't have any prior experience with pipeline syntax.

I don't believe there is a way from RB to re-trigger an integration. Re-triggering it from Jenkins with the same build parameters would work.

Hi André,
I'll field some of these questions and suggestions.

Configuration
It's important that we allow for configuration to have their own RB token and URL, but it would be nice to make it easy to reuse existing ones that were already set. There are companies out there with multiple Review Board servers in use.

API Tokens and users
It's easy to create a custom user, but I'm not sure which special rights you're referring to.
You can create an API token with a policy restricting the operations it can perform, for instance limiting to POST/PUT requests on certain resources. We should probably include documentation on doing so.
I think we'll want to consider adding support for admin-created API tokens with zero admin capabilities, but it's a larger project.

Polling
No, we can't offer this today. That'd require extra infrastructure for server-side periodic tasks that we're not prepared to set up and wouldn't want to set up for, say, RBCommons.com. It has implications in scaling and performance. For now, it's going to have to be pushing.

Custom text
Technically possible, certainly on the Review Board side. Really, in the end, it's just an API request creating a review, so if you needed something specialized, you could always send your own API requests (and RBTools 1.0 will make this easy). If we're talking a static string, I see no reason not to add that to the integration. I don't know what the VMware Jenkins plugin offers for customization. Can you go into that more? Given licensing issues, we're not looking at their code.

Re-triggering builds
Frankly, this is something we'd want to offer on the Review Board side and not on the integration side, since this isn't Jenkins-specific. For now, I'd like to avoid adding this feature in this integration and instead figure out what makes sense on Review Board. We can have a separate discussion about that.

jenkinsapi bindings
I'll leave this up to James to answer, but more dependencies means a larger install base, potential for future incompatibilities for things like Python versions, and more trouble for our packaging partners. If what we have is working today and is easy to maintain, I'd just say let's stick with it.

Hi Christian and James,
thanks for your response.
Configuration
Yeah, ok, multiple Review Boards should be possible. But I would suggest to let this configuration to be in the jenkins-instance-configuration in "Manage Jenkins" --> "Configure System" as some other plugins adds instance-wide settings to it. Also you could add a "check connection" button as TLS config is often a problem in a company with a custom-PKI. ;-(
There you could add multiple entries for Review Boards with an ID. Otherwise you need to update ALL jobs if you change the user or URL configuration. If this is a pipeline in your source control it is a lot of work to update all repositories.
After that you could use this ID (if ID is empty - use default entry) in any job. Let me show this in a JobDSL example (it's like pipeline).
Using default entry:
job('example') {
    steps {
        reviewboardApplyPatch()
    }
    publishers {
        reviewboardNotifier()
    }
}


Using ID:
job('example') {
    steps {
        reviewboardApplyPatch('mySpecialRB')
    }
    publishers {
        reviewboardNotifier('mySpecialRB')
    }
}


https://jenkinsci.github.io/job-dsl-plugin/#path/job
Or just read .reviewboardrc as a fallback if there is no configuration? Just an idea.
By the way... JobDSL is like pipeline syntax. If your plugin will be in "jenkins plugin store" I will add support to JobDSL plugin. :-)

API Tokens and users
Ok, my words were a little bit misleading. Of course, I could create a user for this.

I mean your wiki entry "Both steps require a Review Board URL as well as an API token. This API token must be an administrator's API token". Would be nice if this could be more fine grained and how to restrict a token to the minimum necessary rights of that "admin token". :-)

Polling
Ok, I understand it is a lot of work. It was just an idea to have better and easier control over started jobs as Review Board will show "running" even the job is already finished just because the job had a misconfiguration and didn't tell Review Board the result. Polling would avoid that problem. But of course.... polling isn't the best one. A mixture of both would be nice. But that, of course, is not a show stopper. :-)

Custom text
I didn't mean a "static text" what will be posted as a review from jenkins. I mean a "static text" for the job that can use environment variables that will be filled with live if the job is finished.

job('example') {
    publishers {
        reviewboardNotifier() {
            text('Link to job: ${ENV_VAR_EXAMPLE}\n State of downstream job: ${ENV_VAR_STATUS_DOWNSTREAM_ABC_EXAMPLE}')
        }
    }
}


We trigger a multijob on jenkins for a review build. That will trigger a lot of downstream job. We use the custom message to provide detailed information of triggered downstream builds as the information are kept by the multijob and can be be used as an env variable.
Yeah, ok, if rbtools 1.0 will support easy review API that would be great. But that would be another configuration of user/token.
Re-triggering builds
Yes, it would just an idea. Of course re-triggering shouldn't be part of a single integration extension but of Review Board at all. :-) If I can manually re-trigger it with the initial STATUS_ID and Review Board accept updates on it, that would be fine.

jenkinsapi bindings
It's ok for me. ;-) It is just a detail of implementation. I just wanted to give a hint that there are a lot of python libs for jenkins. Pure REST is ok, too. :-)

Hey Andre,
The latest revision changes the configuration process. Now you create a server configuration in the "Configure System" page, and each build step simply references the builds in that page. I also changed it so API tokens are stored as credentials, rather than as text. There's still some additional work to be done, but it works. I hope to address your other concerns (posting comments, admin API token, pipeline syntax) in the next couple weeks.

Hi James,
I have just another idea for better configuration handling. ;-)

Maybe you could provide the REVIEWBOARD_URL as an additional job parameter like REVIEWBOARD_REVIEW_ID that will be provided by the calling reviewboard instance. This could be used to automatically fetch the API token from your jenkins "configure system" configuration. I mean just to replace the "explicit id" handling by REVIEWBOARD_URL parameter and just configure "value of REVIEWBOARD_URL --> API-Token". So you don't need any explicit ID mapping in a job config.
What do you think?

Hey Andre,
Yeah that is a much better system. I've added a new parameter REVIEWBOARD_SERVER that fulfils this, so now there's zero configuration required for each individual build step.

If I understand you correctly, this would force users to have jobs with specific names. Is this something we want? Do we want to automatically create these jobs?

If the job name is configurable in the integration's configuration page, then we could support having variables in it that would be replaced. This would be an optional feature, and if the job name is simply "review_build", it wouldn't need a job per branch.

Oh I see! Yeah that'll be a nice feature to have, I'll add this in.

Yes, thanks for explanation! I mean it totally optional!

The latest revision includes two job variables, {repository} and {branch}. Let me know if there is anything else you feel could be added!

Yup that is a possibility. I guess the idea is to handle reviews that have parent reviews which have not yet been submitted?

When a diff is uploaded to a review request, it may have a parent diff associated with it. This is useful if you have this branch structure:

 o [my-branch]
 |
 o [parent-branch]
 |
 | [origin/master]
...


and you want to post just the commit at my-branch for review. In this example, parent-branch is local, so Review Board can't get access to it from the repository. So, RBTools posts the commit at my-branch as the diff to review, and uses the one from origin/master..parent-branch as the parent diff.
In order to then test against this, Jenkins will need that parent diff informatino, if associated with the review request. It would then apply the parent diff first, then apply the normal diff on top of it.

I see, thanks for the info! Do the existing CI integrations handle this use case as well? I don't see any mention of parent diffs in their code, they seem to just fetch and use the latest diffset.

The CircleCI integration uses rbt patch to apply the patch. rbt patch --diff-revision <revision> <review_request> will apply both the parent diff and the diff itself.

 Again... thanks for explanation!

Thanks Barret for pointing me in the right direction! The next revision will use rbt patch rather than the current mechanism of applying a patch.

By the way.... hopefully this is an optional task. We need our own command to patch on different parts. :-)

That's why jenkins-reviewbot of VMWare added that: https://github.com/vmware/jenkins-reviewbot/commit/1c5497ee8c59b98b010f4b2c789855c0ab4b197d

Currently this patching is done via a build step that you would have to add to your job. Thus it is up to the end user to choose whether or not to use it. More advanced users could choose not to use it and instead develop their own method of patching their files, using the Review Board web API to fetch the diff.

Thanks for the comments. I'll address the doc comments in the next revision.

Ah, this is the default indentation for function parameters on a new line in intellij. I'll fix up the styling in the next revision so that it better follows the python style guide.

Yeah that'd be better, it's exactly the same code. I'll ask them what their thoughts are.

Ah, thanks for finding this! This also affects the other CI integrations, so I'll submit a fix for those as well :)

Nope, this is a completely separate user. It's created so we have a unique user that manages the StatusUpdate objects that are created during the integration (these are like the flake8 passed. or JSHint passed. messages attached to review requests).

So this is to account for a race condition when the jenkins-ci user hasn't yet been created, then two review requests are simultaenously published and simultaneously run this bit of code.

Oops! Thanks!

Sure. So in the reviewboard codebase there is an existing class, reviewboard.hostingsvcs.service.URLRequest. rbintegrations.util.urlrequest.URLRequest duplicated all of the functionality of that class. So rather than maintain two classes with the same code, we should just use the existing one. However there is a slight issue that the existing class is an internal class. Eventually it'll be moved to a "public" class, but in the meantime we import that class in this file. Once it's moved we can remove this file and change the other imports in the rbintegrations codebase.

It's an option on Jenkins, so we want to support users who have disabled it (for whatever reason).

Ahh, makes sense. Thanks for all the replies :)

Yeah, that's no longer an issue. However, this integration is currently using basic auth instead of using an API token, so it is still required. Unfortunately I only have about 2 weeks left to finish this project as part of the course I'm taking, so having the option to choose between basic auth and API tokens will have to be a future change (alongside some of your other requests).

I was letting this bubble up, leaving it to callers to interpret what a null serverUrl was. That's ambiguous so now this function throws MalformedURLException, and functions calling this now log an error to the build log.