Add Jenkins CI integration

Review Request #9507 — Created Jan. 27, 2018 and submitted

Information

rbintegrations
master
3d0d2e8...

Reviewers

Jenkins is a continuous integration environment, widely used in industry
for running automated test suites, performing deployments, as well as
many other use cases.

This change adds a Jenkins integration to Review Board. This will enable
users to add this integration and have new review requests trigger
builds on a Jenkins server.

This additionally features a Jenkins plugin. This plugin features two
build steps. The first build step, to occur prior to the main job's
build steps, applies the review request's diff using rbt patch. The
second, post-build step notifies review board of the build's status.

rbintegrations/util/urlrequest.py featured duplicate code from the
reviewboard repository, reviewboard.hostingsvcs.service.URLRequest.
That class is currently an internal class, so rather than importing it
in all files that use it in this repository, for now we'll import it
once in this file. Once URLRequest is refactored in the reviewboard
repo, we can remove the urlrequest.py file and import the original
class.

  • Unit tests have been created and ran for the Jenkins java plugin
    • Run mvn test in the base directory
  • Unit tests have been created and ran for rbintegrations
  • Manual testing of creating reviews, publishing them, triggering
    builds on Jenkins server, and those builds updating the status
    once complete.
Description From Last Updated

You are missing a lot of doc comments on methods.

brenniebrennie

F401 'django.utils.http.urlquote_plus' imported but unused

reviewbotreviewbot

F401 'django.utils.translation.ugettext_lazy as _' imported but unused

reviewbotreviewbot

would be nice if the jobname could use some variables that will be replaced. Example: jobname = '{branch}_review_build' So it …

miserymisery

E123 closing bracket does not match indentation of opening bracket's line

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

F401 'yaml' imported but unused

reviewbotreviewbot

F401 'django.core.urlresolvers.reverse' imported but unused

reviewbotreviewbot

F401 'reviewboard.admin.server.build_server_url' imported but unused

reviewbotreviewbot

is it possible to provide the parent diff also?

miserymisery

F841 local variable 'status_update' is assigned to but never used

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

I don't knwo that we have a style guide for Java, but 8 spaces as a continutation indent seems excessive.

brenniebrennie

This should be a java.net.URL.

brenniebrennie

If reviewBoardURL is a java.net.URL then we can do: URL url = new URL( reviewBoardURL, String.format("/api/review-requests/%s/status-updates/%s/", reviewId, statusId));

brenniebrennie

Comments should be full sentences and end in periods.

brenniebrennie

I think this is inferrable from the SC_OK bit.

brenniebrennie

F401 'base64' imported but unused

reviewbotreviewbot

F841 local variable 'repository' is assigned to but never used

reviewbotreviewbot

Do we want to just use reviewboard.hostingsvcs.service.URLRequest here? We are duplicating logic across projects. This is something we should address …

brenniebrennie

Instead of just calling this "Review Board", can we be more specific? "Review Board plugin for Jenkins"?

daviddavid

This blank line can go away.

daviddavid

This blank line can go away.

daviddavid

Args should be final here.

daviddavid

Args should be final.

daviddavid

final?

daviddavid

Period at the end.

daviddavid

final

daviddavid

Can you put a blank line above this?

daviddavid

Here's a somewhat nicer way to wrap this: ReviewBoardUtils.updateStatusUpdate( new URL(getReviewBoardURL()), getReviewBoardAPIToken(), reviewRequest.getReviewId(), reviewRequest.getStatusUpdateId(), state, description);

daviddavid

final?

daviddavid

final

daviddavid

final

daviddavid

final

daviddavid

final

daviddavid

final

daviddavid

Extra lank line.

daviddavid

final

daviddavid

final

daviddavid

final

daviddavid

final

daviddavid

final

daviddavid

final

daviddavid

final

daviddavid

final

daviddavid

final

daviddavid

final

daviddavid

final

daviddavid

final

daviddavid

final

daviddavid

Needs a space between () and {

daviddavid

This blank line can go away.

daviddavid

Period at the end

daviddavid

This blank line can go away.

daviddavid

final

daviddavid

final

daviddavid

final

daviddavid

final

daviddavid

final

daviddavid

final

daviddavid

final

daviddavid

final

daviddavid

final

daviddavid

final

daviddavid

This blank line can go away.

daviddavid

This blank line can go away.

daviddavid

final

daviddavid

final

daviddavid

Blank lines aren't necessary here.

daviddavid

Type should be "unicode"

daviddavid

Needs a period and a blank line after.

daviddavid

Type should be "unicode"

daviddavid

Needs a period and a blank line after.

daviddavid

Type should be "unicode"

daviddavid

Needs a period and a blank line after.

daviddavid

Type should be "unicode"

daviddavid

Needs a period.

daviddavid

Missing documentation for the patch_info arg.

daviddavid

Docstring for this method should start with a summary line that explains what the function does. Justification like this can …

daviddavid

This blank line isn't necessary.

daviddavid

Missing a docstring

daviddavid

Period at the end.

daviddavid

Period at the end.

daviddavid

Period at the end.

daviddavid

When we have an apostrophe, it's fine to surround the string in double quotes instead of single. Also needs a …

daviddavid

One more blank line here.

daviddavid

I know we might have some docstrings like this, but going forward we're documenting properties like they were attributes instead …

daviddavid

Can you add info in your change description about this part of the change?

daviddavid

this will fail if URLAvatarService is disabled and no jenkins-ci is called at all. :-(

miserymisery

Jenkins?

MA maram

is this still valid since this issue was fixed for jenkins LTS? https://issues.jenkins-ci.org/browse/JENKINS-22474

miserymisery

Can we use "URL" instead of "Url"?

daviddavid

And here.

daviddavid

Should there be logging if there is a MalformedURLException?

JT jtrang

And here.

daviddavid

And here.

daviddavid

Can we use "URL" instead of "Url"?

daviddavid

And here.

daviddavid

And here.

daviddavid

And here.

daviddavid

Can we use "URL" instead of "Url"?

daviddavid

I'm not sure what RB's Java convention is here, but I think credentials.isEmpty() would be more expressive here.

JT jtrang

j before l

daviddavid

Docstrings should have one line, then a blank line, then any additional paragraphs (as necessary).

daviddavid

Summary line of the docstring should be just one line. Additional details can be provided in subsequent paragraphs.

daviddavid

Should be "kwargs" instead of "kwarg"

daviddavid

Blank docstring

JT jtrang

This docstring needs a "Returns:" section explaining the returned value.

daviddavid

We have an improved version of this as djblets.util.decorators.cached_property, which should probably be used instead.

daviddavid

Property docstrings should be documented like attributes rather than methods. So just """The icons used for the integration."""

daviddavid

typo: suppored -> supported

daviddavid

Add a trailing comma.

daviddavid

There are two potentially better ways to wrap this: status_update.description = \ 'failed to communicate with Jenkins' or status_update.description = …

daviddavid

Add a blank line above this.

daviddavid

Please use single quotes instead of double.

daviddavid

Add a blank line above this. Also, the u prefix on the string isn't necessary because you're importing unicode_literals

daviddavid

Please use single quotes instead of double.

daviddavid

So that output is consistent when running the suite, all test docstrings should start with "Testing"

daviddavid

So that output is consistent when running the suite, all test docstrings should start with "Testing"

daviddavid

So that output is consistent when running the suite, all test docstrings should start with "Testing"

daviddavid

Can you add a comment explaining why people shouldn't import this directly? (basically just copy from your review request description).

daviddavid
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

misery
  1. 
      
    1. 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!

    2. 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.

    3. 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.

    4. 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.

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

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

    7. 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!

    8. 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.

    9. 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. :-)

    10. 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.

    11. 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.

    12. 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. :-)

    13. 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.

    14. 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?

    15. 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.

  2. rbintegrations/jenkinsci/api.py (Diff revision 1)
     
     
     
     
     

    would be nice if the jobname could use some variables that will be replaced.

    Example:

    jobname = '{branch}_review_build'
    

    So it would call default_review_build job for default branch.

    1. 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?

    2. 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.

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

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

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

  3. rbintegrations/jenkinsci/integration.py (Diff revision 1)
     
     
     
     
     

    is it possible to provide the parent diff also?

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

    2. 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.

    3. 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.

    4. 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.

    5. 👍 Again... thanks for explanation!

    6. 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.

    7. 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

    8. 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.

  4. 
      
jshephard
jshephard
Review request changed

Change Summary:

This change features the notification part of the Jenkins plugin. Now
this has a complete end-to-end workflow. New review requests will
trigger builds on Jenkins, the builds will apply the patch for that
given review request, and then Jenkins will notify RB of the success
or failure of the build.

Description:

~  

Preliminary work on Jenkins integration. This is enough to have the

~   integration configurable on Review Board, as well as enough to trigger a
~   remote Jenkins build. This doesn't support notifications yet (so builds
~   are always in progress), nor does it have any tests. The code is based
~   off the travis-ci integration and shares a good chunk of it.

  ~

Jenkins is a continuous integration environment, widely used in industry

  ~ for running automated test suites, performing deployments, as well as
  ~ many other use cases.

  ~
  ~

This change adds a Jenkins integration to Review Board. This will enable

  + users to add this integration and have new review requests trigger
  + builds on a Jenkins server.

  +
  +

This additionally features a Jenkins plugin. This plugin features two

  + build steps. The first build step, to occur prior to the main job's
  + build steps, applies the review request's diff using rbt patch. The
  + second, post-build step notifies review board of the build's status.

  +
  +

There is still additional work to be done. Notably, there are no unit

  + tests. There was also the request of having variable in job names (i.e.
  + {branch}), which still needs to be added.

Testing Done:

~  

Minimal testing has been done. This is mostly a POC for triggering Jenkins builds.

  ~

Some testing has been done. All testing has been using a git repository,

  + so I still need to test other repositories to see how they will function.
  + This is mostly a POC for triggering Jenkins builds. Future revisions will
  + feature unit tests.

Commit:

-f5c992e3ec9130330cc6812ae6cbdb0891708681
+3d153125b6425c1e56b0251fabec84b1a41d6e03

Diff:

Revision 3 (+968 -1)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
  1. 
      
  2. You are missing a lot of doc comments on methods.

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

  3. I don't knwo that we have a style guide for Java, but 8 spaces as a continutation indent seems excessive.

    1. 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.

  4. If reviewBoardURL is a java.net.URL then we can do:

    URL url = new URL(
        reviewBoardURL,
        String.format("/api/review-requests/%s/status-updates/%s/", reviewId, statusId));
    
  5. Comments should be full sentences and end in periods.

  6. I think this is inferrable from the SC_OK bit.

  7. rbintegrations/util/urlrequest.py (Diff revision 3)
     
     

    Do we want to just use reviewboard.hostingsvcs.service.URLRequest here? We are duplicating logic across projects.

    This is something we should address with Christian and David.

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

  8. 
      
jshephard
jshephard
david
  1. This looks like a lot of comments, but they're all pretty tiny things (your change is looking so good I reviewed it as if there were no "[WIP]" tag).

  2. plugins/jenkins/pom.xml (Diff revision 5)
     
     

    Instead of just calling this "Review Board", can we be more specific? "Review Board plugin for Jenkins"?

  3. Can you put a blank line above this?

  4. Here's a somewhat nicer way to wrap this:

    ReviewBoardUtils.updateStatusUpdate(
        new URL(getReviewBoardURL()),
        getReviewBoardAPIToken(),
        reviewRequest.getReviewId(),
        reviewRequest.getStatusUpdateId(),
        state,
        description);
    
  5. rbintegrations/jenkinsci/api.py (Diff revision 5)
     
     
     
     
     
     

    Blank lines aren't necessary here.

  6. rbintegrations/jenkinsci/api.py (Diff revision 5)
     
     

    Type should be "unicode"

  7. rbintegrations/jenkinsci/api.py (Diff revision 5)
     
     

    Needs a period and a blank line after.

  8. rbintegrations/jenkinsci/api.py (Diff revision 5)
     
     

    Type should be "unicode"

  9. rbintegrations/jenkinsci/api.py (Diff revision 5)
     
     

    Needs a period and a blank line after.

  10. rbintegrations/jenkinsci/api.py (Diff revision 5)
     
     

    Type should be "unicode"

  11. rbintegrations/jenkinsci/api.py (Diff revision 5)
     
     

    Needs a period and a blank line after.

  12. rbintegrations/jenkinsci/api.py (Diff revision 5)
     
     

    Type should be "unicode"

  13. rbintegrations/jenkinsci/api.py (Diff revision 5)
     
     

    Needs a period.

  14. rbintegrations/jenkinsci/api.py (Diff revision 5)
     
     

    Missing documentation for the patch_info arg.

  15. rbintegrations/jenkinsci/api.py (Diff revision 5)
     
     
     
     

    Docstring for this method should start with a summary line that explains what the function does. Justification like this can go in a second paragraph.

  16. rbintegrations/jenkinsci/api.py (Diff revision 5)
     
     

    This blank line isn't necessary.

  17. rbintegrations/jenkinsci/api.py (Diff revision 5)
     
     

    Missing a docstring

  18. rbintegrations/jenkinsci/forms.py (Diff revision 5)
     
     

    Period at the end.

  19. rbintegrations/jenkinsci/forms.py (Diff revision 5)
     
     

    Period at the end.

  20. rbintegrations/jenkinsci/forms.py (Diff revision 5)
     
     

    Period at the end.

  21. rbintegrations/jenkinsci/forms.py (Diff revision 5)
     
     

    When we have an apostrophe, it's fine to surround the string in double quotes instead of single. Also needs a period at the end.

  22. rbintegrations/jenkinsci/integration.py (Diff revision 5)
     
     

    One more blank line here.

  23. rbintegrations/jenkinsci/integration.py (Diff revision 5)
     
     
     
     
     
     
     

    I know we might have some docstrings like this, but going forward we're documenting properties like they were attributes instead of functions. This can therefore just be "The icons used for the integration" as a single-line docstring.

  24. rbintegrations/util/urlrequest.py (Diff revision 5)
     
     
     
     
     
     
     
     
     

    Can you add info in your change description about this part of the change?

  25. 
      
jshephard
jshephard
misery
  1. 
      
  2. rbintegrations/jenkinsci/integration.py (Diff revision 7)
     
     
     

    this will fail if URLAvatarService is disabled and no jenkins-ci is called at all. :-(

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

  3. 
      
jshephard
jshephard
MA
  1. 
      
  2. rbintegrations/jenkinsci/common.py (Diff revision 9)
     
     

    Question: Is this associated with ReviewBoard's users? So a RB user would have a related jenkins user or are they separate?

    1. 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).

  3. rbintegrations/jenkinsci/common.py (Diff revision 9)
     
     

    Question: When would this happen? If the user is trying to connect with jenkins in another thread? Or if another user is setting up jenkins??

    1. 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.

  4. 
      
MA
  1. 
      
  2. rbintegrations/jenkinsci/forms.py (Diff revision 9)
     
     

    Jenkins?

  3. rbintegrations/util/urlrequest.py (Diff revision 9)
     
     

    Question:
    Can I ask why this code is not needed anymore?

    1. 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.

  4. 
      
MA
  1. 
      
  2. rbintegrations/jenkinsci/tests.py (Diff revision 9)
     
     

    Question:
    Why is not having a CSRF token allowed?

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

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

  3. 
      
jshephard
misery
  1. 
      
  2. rbintegrations/jenkinsci/api.py (Diff revision 10)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    is this still valid since this issue was fixed for jenkins LTS?

    https://issues.jenkins-ci.org/browse/JENKINS-22474

    1. 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).

  3. 
      
jshephard
JT
  1. 
      
  2. Should there be logging if there is a MalformedURLException?

    1. 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.

  3. I'm not sure what RB's Java convention is here, but I think credentials.isEmpty() would be more expressive here.

  4. rbintegrations/jenkinsci/forms.py (Diff revision 11)
     
     

    Blank docstring

  5. 
      
david
  1. This is fantastic. I have a few small, very trivial comments, but I'd say this is amount 99% there.

  2. rbintegrations/jenkinsci/api.py (Diff revision 11)
     
     
     

    j before l

  3. rbintegrations/jenkinsci/api.py (Diff revision 11)
     
     
     

    Docstrings should have one line, then a blank line, then any additional paragraphs (as necessary).

  4. rbintegrations/jenkinsci/api.py (Diff revision 11)
     
     
     

    Summary line of the docstring should be just one line. Additional details can be provided in subsequent paragraphs.

  5. rbintegrations/jenkinsci/forms.py (Diff revision 11)
     
     

    Should be "kwargs" instead of "kwarg"

  6. rbintegrations/jenkinsci/forms.py (Diff revision 11)
     
     

    This docstring needs a "Returns:" section explaining the returned value.

  7. rbintegrations/jenkinsci/integration.py (Diff revision 11)
     
     

    We have an improved version of this as djblets.util.decorators.cached_property, which should probably be used instead.

  8. rbintegrations/jenkinsci/integration.py (Diff revision 11)
     
     

    Property docstrings should be documented like attributes rather than methods. So just """The icons used for the integration."""

  9. rbintegrations/jenkinsci/integration.py (Diff revision 11)
     
     

    typo: suppored -> supported

  10. rbintegrations/jenkinsci/integration.py (Diff revision 11)
     
     

    Add a trailing comma.

  11. rbintegrations/jenkinsci/integration.py (Diff revision 11)
     
     
     

    There are two potentially better ways to wrap this:

    status_update.description = \
        'failed to communicate with Jenkins'
    

    or

    status_update.description = ('failed to communicate with '
                                 'Jenkins.')
    
  12. rbintegrations/jenkinsci/integration.py (Diff revision 11)
     
     

    Add a blank line above this.

  13. rbintegrations/jenkinsci/integration.py (Diff revision 11)
     
     

    Please use single quotes instead of double.

  14. rbintegrations/jenkinsci/integration.py (Diff revision 11)
     
     

    Add a blank line above this.

    Also, the u prefix on the string isn't necessary because you're importing unicode_literals

  15. rbintegrations/jenkinsci/integration.py (Diff revision 11)
     
     

    Please use single quotes instead of double.

  16. rbintegrations/jenkinsci/tests.py (Diff revision 11)
     
     

    So that output is consistent when running the suite, all test docstrings should start with "Testing"

  17. rbintegrations/jenkinsci/tests.py (Diff revision 11)
     
     

    So that output is consistent when running the suite, all test docstrings should start with "Testing"

  18. rbintegrations/jenkinsci/tests.py (Diff revision 11)
     
     

    So that output is consistent when running the suite, all test docstrings should start with "Testing"

  19. rbintegrations/util/urlrequest.py (Diff revision 11)
     
     

    Can you add a comment explaining why people shouldn't import this directly? (basically just copy from your review request description).

  20. 
      
jshephard
jshephard
misery
  1. 
      
  2. Any news here? Is there a roadmap when it will be merged?

    1. I'd also really like to know what the ETA of this plugin being merged is as I'd like to use it for our jenkins setup. Is James the only one who can merge it?

    2. We're working on getting the jenkins plugin accepted to their hosting repository. Once that's done, we'll land the integration, planning to ship it in rbintegrations 2.0 (shipping alongside Review Board 4.0, in beta soon).

  3. 
      
jshephard
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (e0a36e7)
Loading...