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

Include the Checks API in build status #2102

Merged
merged 35 commits into from May 13, 2019
Merged

Conversation

@smashwilson
Copy link
Member

@smashwilson smashwilson commented Apr 26, 2019

Please be sure to read the contributor's guide to the GitHub package before submitting any pull requests.

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • Suggestion: You can use checklists to keep track of progress for the sections on metrics, tests, documentation, and user research.

Description of the Change

My goal is to account for check suites and check runs everywhere we display information about a PR or commit's status, to bring us in sync with the information that dotcom provides.

  • CheckSuites and CheckRuns are both paginated collections. I'm following the pattern we introduced to deal with nested pagination in reviews and review comments to introduce Accumulators for these.
  • I'm adding a BuildStatus model to unify the information we derive from CheckSuite status and conclusions and build contexts.
  • Change the "Build Status" tab within PullRequestDetailView to show check suites and check runs.
    • The existing PullRequestStatusContextView needs to be adjusted to use BuildStatus.
    • New CheckSuiteView and CheckRunView components render status rows from check suites and runs.
    • PullRequestStatusesView needs to render the check suite accumulator to collect and render check suite data.
    • Mess around with CSS to make check suite and check run status rows look reasonable.
    • CheckRuns include a summary field that may contain raw Markdown. Render and sanitize it.
  • Change the IssueishListView to account for check results when rendering its status summary icon.
    • Use BuildStatus within the Issueish model used by the IssueishListView.
    • Fetch accumulated check suite and run data along with issueish search results.
  • Don't show the "unverified/unknown" status in the PR detail item before check runs arrive.

Screenshots

status checks

Alternate Designs

TODO

Benefits

Repositories configured to report advanced CI information with the Checks API will be accounted for correctly in the GitHub tab and pull request detail items.

Possible Drawbacks

If you have many check suites being reported on each commit, this may chew through your rate limit pretty quickly.

Applicable Issues

Fixes #1818.

Metrics

N/A

Tests

Recent commits in this repository have both legacy commit build statuses (from CodeCov) and check suite statuses (from Azure Pipelines). I've been using PRs within in this repo to verify that things look correct.

Documentation

N/A

Release Notes

  • Build statuses reported through the Checks API are included in the GitHub tab and pull request detail items.

User Experience Research (Optional)

N/A

@smashwilson smashwilson self-assigned this Apr 26, 2019
@smashwilson smashwilson force-pushed the aw/include-status-checks branch from b780f12 to 6ea63ca Apr 26, 2019
@smashwilson smashwilson added this to In progress in Sprint : 4 April 2019 - 8 May 2019 : v0.29.0 via automation Apr 26, 2019
@codecov
Copy link

@codecov codecov bot commented Apr 26, 2019

Codecov Report

Merging #2102 into master will decrease coverage by 0.18%.
The diff coverage is 98.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2102      +/-   ##
==========================================
- Coverage   92.72%   92.53%   -0.19%     
==========================================
  Files         207      212       +5     
  Lines       12053    12171     +118     
  Branches     1764     1779      +15     
==========================================
+ Hits        11176    11263      +87     
- Misses        877      908      +31
Impacted Files Coverage Δ
lib/containers/current-pull-request-container.js 100% <ø> (ø) ⬆️
lib/controllers/issueish-detail-controller.js 100% <ø> (ø) ⬆️
lib/containers/issueish-detail-container.js 100% <ø> (ø) ⬆️
lib/controllers/issueish-list-controller.js 100% <ø> (ø) ⬆️
lib/containers/issueish-search-container.js 95.83% <ø> (ø) ⬆️
lib/views/pr-detail-view.js 100% <ø> (ø) ⬆️
lib/helpers.js 88.47% <100%> (-1.53%) ⬇️
lib/views/check-run-view.js 100% <100%> (ø)
.../containers/accumulators/check-runs-accumulator.js 100% <100%> (ø)
lib/models/build-status.js 100% <100%> (ø)
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6569916...dc215bc. Read the comment docs.

Loading

@smashwilson smashwilson marked this pull request as ready for review Apr 29, 2019
@smashwilson smashwilson requested a review from Apr 29, 2019
Copy link
Contributor

@kuychaco kuychaco left a comment

Looks good! Excited to have more dotcom goodness in our extension with this addition 😁

Left a few of questions for ya. Unlikely that anything is blocking.

Loading

Loading
lib/controllers/issueish-detail-controller.js Show resolved Hide resolved
Loading
lib/models/issueish.js Outdated Show resolved Hide resolved
Loading
lib/models/issueish.js Outdated Show resolved Hide resolved
Loading
lib/views/issueish-list-view.js Outdated Show resolved Hide resolved
Loading
@smashwilson smashwilson added this to In progress in Release : 9 May 2019 - 5 June 2019 : v0.30.0 via automation May 8, 2019
@kuychaco kuychaco mentioned this pull request May 10, 2019
9 tasks
Copy link
Contributor

@vanessayuenn vanessayuenn left a comment

again, just a couple of questions; nothing blocking. Excited to ship this!

Loading

}

if (props.markdown && props.markdown !== state.lastMarkdown) {
return {html: renderMarkdown(props.markdown), lastMarkdown: props.markdown};
Copy link
Contributor

@vanessayuenn vanessayuenn May 13, 2019

Choose a reason for hiding this comment

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

💯

Loading

@@ -9,6 +9,8 @@ export const LINE_ENDING_REGEX = /\r?\n/;
export const CO_AUTHOR_REGEX = /^co-authored-by. (.+?) <(.+?)>$/i;
export const PAGE_SIZE = 50;
export const PAGINATION_WAIT_TIME_MS = 100;
export const CHECK_SUITE_PAGE_SIZE = 10;
export const CHECK_RUN_PAGE_SIZE = 20;
Copy link
Contributor

@vanessayuenn vanessayuenn May 13, 2019

Choose a reason for hiding this comment

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

What's the reasoning behind using these two numbers, instead of reusing the already defined PAGE_SIZE?

Loading

Copy link
Member Author

@smashwilson smashwilson May 13, 2019

Choose a reason for hiding this comment

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

Oh, yeah. The page sizes we use in the query influences the rate-limiting cost associated with any given GraphQL query (as opposed to the number of results that are actually returned). It gets pretty complex, I'm not sure I fully understand it yet. But keeping our page sizes lower in cases when we probably don't need them gives us more "wiggle room" in our queries to add stuff later. I'm (unscientifically) betting here that most check suite and check run counts are below these numbers anyway.

What would be ideal, I think, is splitting out our page sizes for each resource type, and deriving a value from real dotcom stats, to target a generous 90% of real queries fitting in a single page (say).

Loading

@smashwilson smashwilson merged commit 04db109 into master May 13, 2019
14 checks passed
Loading
Release : 9 May 2019 - 5 June 2019 : v0.30.0 automation moved this from In progress to Merged May 13, 2019
@smashwilson smashwilson deleted the aw/include-status-checks branch May 13, 2019
@smashwilson smashwilson mentioned this pull request Jul 18, 2019
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Linked issues

Successfully merging this pull request may close these issues.

3 participants