-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Bitbucket] #1509 - Add Bitbucket Pipelines service #1510
[Bitbucket] #1509 - Add Bitbucket Pipelines service #1510
Conversation
Generated by 🚫 dangerJS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this contribution and for adding tests. This looks like it is in really good shape already but I've left a couple of minor comments.
server.js
Outdated
var user = match[1]; // eg, atlassian | ||
var repo = match[2]; // eg, adf-builder-javascript | ||
var branch = match[3] || 'master'; // eg, development | ||
var format = match[4]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When adding new code could you use the ES6 style let
and const
rather than var
please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do, thanks. I tried to stick to the existing style, but happy to use ES6.
server.js
Outdated
if (err != null) { | ||
badgeData.text[1] = 'inaccessible'; | ||
sendBadge(format, badgeData); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block here and the later sections where you're checking for if (res.statusCode !== 200)
and if (res.statusCode === 404)
are really common so we have now added a helper. You can simplify all that to:
Lines 6515 to 6518 in 75503ec
if (checkErrorResponse(badgeData, err, res)) { | |
sendBadge(format, badgeData); | |
return; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I'll use that. I'll make the changes later today.
I replaced the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brilliant - thanks. I think this is good to merge but one of the maintainers will need to do a final check + merge.
Thanks for your contribution, Changes look good to me, Thanks @chris48s for reviewing! |
Thanks @chris48s and @RedSparr0w for taking care of this so quickly! |
@RedSparr0w how often do you deploy? Do you have a rough ETA on when this would land in production? |
On mobile right now, but this comment here should provide some info: Hopefully shouldn't be too far off the next deployment though. |
Adding support for Bitbucket Pipelines builds: #1509 .
The format is the same as for the other Bitbucket badges:
/bitbucket/pipelines/<user>/<repo>.svg
and
/bitbucket/pipelines/<user>/<repo>/<branch>.svg
The implementation is looking at the last completed build result (ignoring builds that are about to start or in progress). It supports all current Pipelines result states:
The tests cover 100% of the added lines.
Let me know what you think, thanks a lot!