Skip to content
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

[GitHub] Issue and pull request detail and check state #1114

Merged
merged 11 commits into from
Oct 2, 2017
Merged

[GitHub] Issue and pull request detail and check state #1114

merged 11 commits into from
Oct 2, 2017

Conversation

paulmelnikow
Copy link
Member

@paulmelnikow paulmelnikow commented Sep 29, 2017

This adds badges for Github issues and pull requests. You can display the state, title, username, number of comments, age, time since last update, and state of checks.

Blocked on #1112 (see FIXMEs).

Provides an endpoint the Shields CI can use to fetch PR titles for #979 and resolves #1011.

@paulmelnikow paulmelnikow added developer-experience Dev tooling, test framework, and CI service-badge Accepted and actionable changes, features, and bugs labels Sep 29, 2017
.expectJSONTypes(Joi.object().keys({ name: 'updated', value: validDateString }));

t.create('github pull request check state')
.get('/pulls/checks/s/badges/shields/1110.json')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make this /status/s/pulls/badges/shields/1110.json which might be nicer for adding check state of branches + commits, as in #774.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated: c727f2f.

server.js Outdated
// GitHub issue detail integration.
camp.route(/^\/github\/(?:issues|pulls)\/detail\/(s|title|u|label|comments|age|last-update)\/([^\/]+)\/([^\/]+)\/(\d+)\.(svg|png|gif|jpg|json)$/,
cache((queryParams, match, sendBadge, request) => {
const stateColor = s => ({ open: '2cbe4e', closed: 'cb2431', merged: '6f42c1' }[s]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to re-define the function ever time this route is called, which is not really necessary as it's entirely static. Instead, pull this function out and make it a regular function in this file (or in one of the utility modules).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 448e88c.

@paulmelnikow
Copy link
Member Author

Service tests passing locally with this patch that works around a bug in our logic for token rate limit handling.

@paulmelnikow paulmelnikow merged commit bb66a99 into badges:master Oct 2, 2017
@paulmelnikow paulmelnikow deleted the github-issue-info branch October 2, 2017 17:26
paulmelnikow added a commit that referenced this pull request Oct 6, 2017
Use the endpoint on img.shields.io added in #1114 to fetch the PR title.

This sidesteps the authentication requirement, and helps with #979 – except for PRs that need to run the Github service tests, which will need a separate solution.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Dev tooling, test framework, and CI service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open/Closed State badge for github issues.
2 participants