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 fixes #418

Merged
merged 2 commits into from
Feb 5, 2018
Merged

GitHub fixes #418

merged 2 commits into from
Feb 5, 2018

Conversation

bkeepers
Copy link
Contributor

@bkeepers bkeepers commented Feb 4, 2018

Errors and 4xx responses from GitHub are currently being obscured since #400 by this error:

    TypeError: Cannot read property 'meta' of undefined
        42 |   octokit.hook.after('request', (result, options) => {
        43 |     const {method, url, headers, ...params} = options
      > 44 |     const msg = `GitHub request: ${method} ${url} - ${result.meta.status}`
        45 |     logger.debug({params}, msg)
        46 |   })
        47 |   octokit.paginate = paginate.bind(null, octokit)

        at EnhancedGitHubClient.octokit.hook.after (lib/github.js:44:62)
        at invokeAfterHook (node_modules/before-after-hook/lib/register.js:96:12)
            at Array.map (<anonymous>)
        at node_modules/before-after-hook/lib/register.js:60:35

c69f2df updates the error hook to re-throw errors after logging them.

There's also a small tweak here to allow a customer limiter to be passed in at load time to prevent the tests from running very slowly.

Fixes this error for 4xx responses:

    TypeError: Cannot read property 'meta' of undefined
        42 |   octokit.hook.after('request', (result, options) => {
        43 |     const {method, url, headers, ...params} = options
      > 44 |     const msg = `GitHub request: ${method} ${url} -
${result.meta.status}`
        45 |     logger.debug({params}, msg)
        46 |   })
        47 |   octokit.paginate = paginate.bind(null, octokit)

        at EnhancedGitHubClient.octokit.hook.after (lib/github.js:44:62)
        at invokeAfterHook
(node_modules/before-after-hook/lib/register.js:96:12)
            at Array.map (<anonymous>)
        at node_modules/before-after-hook/lib/register.js:60:35

    at Object.<anonymous> (node_modules/expect/build/index.js:179:51)
        at Generator.throw (<anonymous>)
    at step (node_modules/expect/build/index.js:43:727)
    at node_modules/expect/build/index.js:43:926
@bkeepers bkeepers requested a review from gr2m February 4, 2018 18:15
@codecov
Copy link

codecov bot commented Feb 4, 2018

Codecov Report

Merging #418 into master will increase coverage by 1.1%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #418     +/-   ##
=========================================
+ Coverage   93.86%   94.96%   +1.1%     
=========================================
  Files          19       19             
  Lines         277      278      +1     
  Branches       29       30      +1     
=========================================
+ Hits          260      264      +4     
+ Misses         16       13      -3     
  Partials        1        1

Copy link
Member

@JasonEtco JasonEtco left a comment

Choose a reason for hiding this comment

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

LGTM

@bkeepers bkeepers merged commit 72d6911 into master Feb 5, 2018
@bkeepers bkeepers deleted the github-fixes branch February 5, 2018 15:28
bkeepers added a commit that referenced this pull request Mar 15, 2018
…5.5.0

* origin/master: (39 commits)
  chore(package): update nock to version 9.2.0 (#460)
  chore(package): update raven to version 2.4.2 (#459)
  chore(package): update bottleneck to version 2.2.0 (#458)
  Docs: encourage people to add their app to the website (#454)
  fix(package): update @octokit/rest to version 15.1.9
  chore(package): update semantic-release to version 15.0.0 (#452)
  chore(package): update eslint to version 4.18.2 (#455)
  Docs: Add error tracking to contents (#453)
  Update config.yml (#431)
  Raise an error if there are several .pem files (#441)
  fix(package): update @octokit/rest to version 15.1.7
  docs(README): typo (#449)
  docs: Add Summer of Code section to the README (#448)
  Update node-github => octokit/rest.js in docs (#445)
  chore(package): Update standard to the latest version 🚀 (#436)
  Lock to specific version of express-async-errors (#432) (#444)
  docs: Summer of Code doc (#435)
  fix(package): update @octokit/rest to version 14.0.9
  Update docs about glitch deployment (#421)
  fix: Re-throw errors from octokit (#418)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants