[Bitbucket]: Pull Requests: Support Bitbucket Server

[nlowe]

Add support for counting pull requests from a bitbucket server instance as requested in #1757. I didn't do build status since that can be handled by other badges.

Fixes #1757

[shields-ci]

	Warnings
⚠️	📚 Remember to ensure any changes to serverSecrets in services/bitbucket/bitbucket-pull-request.service.js are reflected in the server secrets documentation
⚠️	📚 Remember to ensure any changes to serverSecrets in services/bitbucket/bitbucket-test-helpers.js are reflected in the server secrets documentation

	Messages
📖	✨ Thanks for your contribution to Shields, @nlowe!

Generated by 🚫 dangerJS against 1327bce

[calebcartwright]

Thanks for your contribution to Shields! FYI I modified the name of the PR to put the service into brackets so the corresponding service tests would be run as part of the cheks

[calebcartwright]

My gut feel is that we should strive to have a single service implementation that can support:

• public repos in BB
• private repos in BB (via self-hosted Shields instance)
• repos in BBS (via main Shields.io service for public, internet-accessible BBS instances and via self-hosted Shields instance for private/internal BBS instances)

Specifically, I'd like to know if we can have a single Bitbucket PR Service implementation (in bitbucket-pull-request.service.js) that can support all of those options above.

It's a fairly common pattern we have in other services and I'd be surprised if we couldn't do the same here. The path can be enhanced to support different options, the handle/fetch functions can be updated accordingly to build and use the right url, options, etc, when required multiple schemas can be defined/used, etc.

I noticed that the API we're currently using for PRs on BB is relatively similar to the API that is being added here for BB server:

BB: /api/2.0/repositories/${user}/${repo}/pullrequests/
BBS: rest/api/1.0/projects/${project}/repos/${repo}/pull-requests

Given the usage of 1.0 I'm curious what the API version breakdown is between major BBS releases. For example does the 1.0 API spec still exist on BBS 5.x, 4.x, etc. as well as whether the 2.0 API spec exists on BBS (and starting with which major verison)

[nlowe]

Yeah that makes sense. Even though Bitbucket Server "versions" its API, it looks like they never change the version (5.16.0, released a month ago, still lists 1.0 as the newest according to their documentation).

As for unifying the service URL for both versions, I'm not familiar enough with the project to know how to add an optional parameter (thinking ${proto}://${host} here). Would that be best added as a query parameter?

[calebcartwright]

As for unifying the service URL for both versions, I'm not familiar enough with the project to know how to add an optional parameter (thinking ${proto}://${host} here). Would that be best added as a query parameter?

So the first thing we want to make sure we do here is ensure we don't break anyone using the existing PR badges for BB. Currently those folks are using the paths:

/bitbucket/pr/:user/:repo (open PRs)
/bitbucket/pr-raw/:user/:repo (all PRs)

Therefore the route would need to support that pattern as well as the new type of pattern we want to add to support BBS. I'd think we'd want to have something like:

/bitbucket/pr/:user/:repo for PR Counts from BB and then support something like /bitbucket/pr/:protocol/:hostAndPath/:project/:repo for BBS

Obviously that regex starts to get a little tricky 😄 In similar instances with other services I've had to use format and capture (instead of pattern) in the route to support these sorts of scenarios. I haven't actually tested this regex, but we should be able to do something like the below for the route (or whatever the valid regex is for the format):

static get route() {
  return {
    base: `bitbucket/${routePrefix}`,
    format: '(?:(http|https)/(.+)/)?([^/]+)/([^/]+)',
    capture: [ 'protocol', 'hostAndPath', 'user', 'repo'],
  }
}

Note in the base the routePrefix is coming from a function defined in the existing implementation that just allows for both bitbucket/pr/.. and bitbucket/pr-raw/.. paths. Also the hostAndPath will support BBS deploys like: https://myinternal.bitbucketserver.com:8000/foo. User would be the same as project as far as the regex is concerned so I think using a single capture group & name there would be best (the examples shown to end users can use :user and :project respectively as needed regardless)

With that kind of route we could update the handle signature to take in the parameters and based on those parameters we could determine if we're targeting BBS or BB and act accordingly :

async handle({ protocol, hostAndPath, user, repo }) {
  let usingBitbucketServer = false
  let url = `https://bitbucket.org/api/2.0/repositories/${user}/${repo}/pullrequests/`
  if (hostAndPath) {
    usingBitbucketServer = true
    url = `${protocol}://${hostAndPath}/rest/api/1.0/projects/${user}/repos/${repo}/pull-requests`
  }
  ...
}

That's obviously a little raw and in need of some cleanup, but I think it conveys the idea well enough

[nlowe]

@calebcartwright Thanks for all the help. I finally had some time to circle back to this and was porting it from a legacy service I had created earlier in the year. I'll take a look at your suggestions tomorrow if I have some time.

[nlowe]

@calebcartwright I was able combine the new service with the existing one. I also followed your suggestions and used the object-style query strings, basic auth, and error code handlers. PTAL when you get a chance.

(Tests failed because the service name in the PR title is no longer correct. I've fixed it but I don't have permissions to restart the CI builds that failed.)

[calebcartwright]

👍 thanks, will review shortly

[calebcartwright]

This is looking great! I've added a couple final comments/requests inline below but I think this is nearly done

[calebcartwright]

Thanks again for this. Someone should come along shortly and merge this (note they may have some additional feedback/requested changes as well), but it LGTM.

The last thing I'll add is that you may want to consider adding some additional tests for the BBS paths (like the HTTP 40X response codes, etc.) similar to the tests for BB that validate those similar scenarios

[nlowe]

@calebcartwright Thanks for all the help, I added extra tests for the other status codes. Is there an easy way to mock out the credentials so I can test the presence / absence of the basic auth header? It's the only test cases I think I'm missing.

[calebcartwright]

@calebcartwright Thanks for all the help, I added extra tests for the other status codes. Is there an easy way to mock out the credentials so I can test the presence / absence of the basic auth header? It's the only test cases I think I'm missing.

Great question! We do have a pattern that allows for stubbing the serverSecret values as part of the setup/teardown process of the test to cover those scenarios. One example is here in the Jira service

[nlowe]

Do you guys want this squashed?

[nlowe]

I think that's everything, although I need to read through #2626 to see if I have to migrate over any of the secrets we added.

[nlowe]

Did the format of examples change? CI is failing: Yup, in 89113ee. Fix incoming.

[nlowe]

@paulmelnikow @calebcartwright Let me know if there's anything else that needs tweaked, I think this is ready for merging.

[calebcartwright]

Thanks @nlowe! giving this a final look over now

[calebcartwright]

Thanks for all your work on this!

[shields-deployment]

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch: