Fix for platformResultsPreMapper/concat problem #355

ashfurrow · 2018-08-16T21:57:53Z

So it took looking at the Danger and Peril codebases side-by-side to realize that we've got a subtle type bug. The Platform type expects an optional platformResultsPreMapper, but Peril was setting that attribute to be a function that returns an optional function. The fix was to move the tertiary expression outside of the platform const definition, to run that check before creating the Platform object (instead of running the check when the platformResultsPreMapper function is called, returning a function where pre-mapper results are expected to be returned).

I've tested and verified this works: ashfurrow/test-repo#13

Fixes #351. Fixes ashfurrow/peril-settings#8.

jlengstorf · 2018-08-17T01:47:52Z

I tried this out and it looks like it gets past the original error, but hits another.

Aug 16 18:43:36 peril-gatsbyjs app/web.1: info: ## issues.opened on unknown on gatsbyjs/peril-gatsbyjs 
Aug 16 18:43:36 peril-gatsbyjs app/web.1: info:    1 run needed: org/labeler.ts 
Aug 16 18:43:37 peril-gatsbyjs app/web.1: 2018-08-17T01:43:36.770Z danger:transpiler:setup Does not have Babel 7 TypeScript set up 
Aug 16 18:43:37 peril-gatsbyjs heroku/router: at=info method=POST path="/webhook" host=peril-gatsbyjs.herokuapp.com request_id=075780b1-40f1-4c2d-b633-741d47637343 fwd="192.30.252.36" dyno=web.1 connect=1ms service=1385ms status=200 bytes=1466 protocol=https 
Aug 16 18:43:38 peril-gatsbyjs app/web.1: Unable to evaluate the Dangerfile 
Aug 16 18:43:38 peril-gatsbyjs app/web.1: info: [runner] - Commenting, with results:  
Aug 16 18:43:38 peril-gatsbyjs app/web.1: mds: 1 
Aug 16 18:43:38 peril-gatsbyjs app/web.1: messages: 0 
Aug 16 18:43:38 peril-gatsbyjs app/web.1: warns: 0 
Aug 16 18:43:38 peril-gatsbyjs app/web.1: fails: 1 
Aug 16 18:43:38 peril-gatsbyjs app/web.1: 2018-08-17T01:43:37.723Z danger:executor Got Results back, current settings { dangerID: 'peril', 
Aug 16 18:43:38 peril-gatsbyjs app/web.1:   jsonOnly: false, 
Aug 16 18:43:38 peril-gatsbyjs app/web.1:   stdoutOnly: false, 
Aug 16 18:43:38 peril-gatsbyjs app/web.1:   verbose: false, 
Aug 16 18:43:38 peril-gatsbyjs app/web.1:   accessTokenIsGitHubApp: true } 
Aug 16 18:43:38 peril-gatsbyjs app/web.1: 2018-08-17T01:43:37.725Z danger:executor Running platformResultsPreMapper: function () { [native code] } 
Aug 16 18:43:38 peril-gatsbyjs app/web.1: 2018-08-17T01:43:37.727Z danger:GitHub::Checks Getting PR details for checks 
Aug 16 18:43:38 peril-gatsbyjs app/web.1: 2018-08-17T01:43:37.729Z danger:GitHubAPI Sending:  https://api.github.com/repos/gatsbyjs/peril-gatsbyjs/pulls/30 { 'Content-Type': 'application/json', 
Aug 16 18:43:38 peril-gatsbyjs app/web.1:   Authorization: 'token v1.[redacted]', 
Aug 16 18:43:38 peril-gatsbyjs app/web.1:   Accept: 'application/vnd.github.machine-man-preview+json' } 
Aug 16 18:43:38 peril-gatsbyjs app/web.1: 2018-08-17T01:43:37.740Z agenda:internal:processJobs starting to process jobs 
Aug 16 18:43:38 peril-gatsbyjs app/web.1: 2018-08-17T01:43:37.740Z agenda:internal:processJobs queuing up job to process: [runDangerfile] 
Aug 16 18:43:38 peril-gatsbyjs app/web.1: 2018-08-17T01:43:37.740Z agenda:internal:processJobs job [runDangerfile] lock status: shouldLock = true 
Aug 16 18:43:38 peril-gatsbyjs app/web.1: 2018-08-17T01:43:37.740Z agenda:internal:_findAndLockNextJob _findAndLockNextJob(runDangerfile, [Function], cb) 
Aug 16 18:43:38 peril-gatsbyjs app/web.1: Request failed [404]: https://api.github.com/repos/gatsbyjs/peril-gatsbyjs/pulls/30 
Aug 16 18:43:38 peril-gatsbyjs app/web.1: Response: { 
Aug 16 18:43:38 peril-gatsbyjs app/web.1:   "message": "Not Found", 
Aug 16 18:43:38 peril-gatsbyjs app/web.1:   "documentation_url": "https://developer.github.com/v3/pulls/#get-a-single-pull-request" 
Aug 16 18:43:38 peril-gatsbyjs app/web.1: } 
Aug 16 18:43:38 peril-gatsbyjs app/web.1: error: Error:  Could not get PR Metadata for repos/gatsbyjs/peril-gatsbyjs/pulls/30 
Aug 16 18:43:38 peril-gatsbyjs app/web.1: /app/out/peril.js:25 
Aug 16 18:43:38 peril-gatsbyjs app/web.1:         throw reason; 
Aug 16 18:43:38 peril-gatsbyjs app/web.1:         ^ 
Aug 16 18:43:38 peril-gatsbyjs app/web.1: Could not get PR Metadata for repos/gatsbyjs/peril-gatsbyjs/pulls/30

Did I do something to break Babel?

Additionally, this is an issue payload, but it's trying to hit the /pulls endpoint. It fires on issue.opened, so I'm not sure how that's happening.

Is that related to this, or is something else going on?

ashfurrow · 2018-08-17T11:50:35Z

@jlengstorf can you clarify if this most recent error uses the SKIP_CHECKS_SUPPORT env var? The linked-to example issue is also an issue.opened payload so I'm a bit confused.

jlengstorf · 2018-08-17T22:42:40Z

@ashfurrow I tried it with and without SKIP_CHECKS_SUPPORT — something appears to have gone very wrong with our install and I'm not sure how to fix it. 😅

Another error we started seeing: gatsbyjs/gatsby#7409 (comment)

jlengstorf · 2018-08-18T01:56:04Z

@ashfurrow I went ahead and rolled our installation back to avoid these bugs. The fix here gets around the original issue, but now we get:

2018-08-18T01:51:26.461534+00:00 app[web.1]: error: Error:  TypeError: Cannot read property 'id' of undefined
2018-08-18T01:51:26.461552+00:00 app[web.1]: at Executor.<anonymous> (/app/node_modules/danger/distribution/runner/Executor.js:441:92)
2018-08-18T01:51:26.461554+00:00 app[web.1]: at step (/app/node_modules/danger/distribution/runner/Executor.js:32:23)
2018-08-18T01:51:26.461556+00:00 app[web.1]: at Object.next (/app/node_modules/danger/distribution/runner/Executor.js:13:53)
2018-08-18T01:51:26.461557+00:00 app[web.1]: at /app/node_modules/danger/distribution/runner/Executor.js:7:71
2018-08-18T01:51:26.461559+00:00 app[web.1]: at new Promise (<anonymous>)
2018-08-18T01:51:26.461561+00:00 app[web.1]: at __awaiter (/app/node_modules/danger/distribution/runner/Executor.js:3:12)
2018-08-18T01:51:26.461563+00:00 app[web.1]: at Executor.deleteInlineComment (/app/node_modules/danger/distribution/runner/Executor.js:438:16)
2018-08-18T01:51:26.461565+00:00 app[web.1]: at Executor.<anonymous> (/app/node_modules/danger/distribution/runner/Executor.js:291:51)
2018-08-18T01:51:26.461566+00:00 app[web.1]: at step (/app/node_modules/danger/distribution/runner/Executor.js:32:23)
2018-08-18T01:51:26.461568+00:00 app[web.1]: at Object.next (/app/node_modules/danger/distribution/runner/Executor.js:13:53)
2018-08-18T01:51:26.461570+00:00 app[web.1]: at fulfilled (/app/node_modules/danger/distribution/runner/Executor.js:4:58)
2018-08-18T01:51:26.461572+00:00 app[web.1]: at <anonymous>
2018-08-18T01:51:26.461574+00:00 app[web.1]: at process._tickCallback (internal/process/next_tick.js:182:7)
2018-08-18T01:51:26.462022+00:00 app[web.1]: /app/out/peril.js:25

No idea what's going on, so I guess I need to wait for @orta to get back before upgrading again.

Thanks again for your help!

orta · 2018-08-20T21:45:34Z

OK, I know what the issue is about (WRT the mysterious PR request) - it's because the level of abstraction for Danger is only PRs the checks API is used on every request, but because when using danger on an issue we support communicating using Danger ( e.g. fail, not via danger.github.api.createComment... ) then Peril currently doesn't take that into account.

Once I get some time with a computer at home, I'll take a look. Things got a bit rough from thursday, so I'm not quite in a space to say when I can do it - likely this week/weekend

jlengstorf · 2018-08-20T23:19:30Z

@orta hey, no rush at all; I'll stop pinging you. So sorry for your loss.

orta · 2018-08-26T13:13:30Z

OK, I've taken a look and I think I've got it with #359 - the key is that nullfunc, we don't need it, just needed it to be totally falsy. I set myself up to also have a danger file running on issues for danger so it should be accounted for in general

ashfurrow · 2018-08-26T14:57:58Z

Nice! I'll pull down and test it on my end.

ashfurrow · 2018-08-26T15:04:22Z

Worked fine for me, thanks Orta: ashfurrow/test-repo#14 (comment)

Attempted fix for platformResultsPreMapper.

e6b2390

ashfurrow mentioned this pull request Aug 16, 2018

Error after updating Peril #351

Open

orta mentioned this pull request Aug 26, 2018

Mega grabbag PR #359

Merged

peril-staging bot closed this in #359 Aug 26, 2018

ashfurrow deleted the platformResultsPreMapper-fix branch August 26, 2018 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for platformResultsPreMapper/concat problem #355

Fix for platformResultsPreMapper/concat problem #355

ashfurrow commented Aug 16, 2018

jlengstorf commented Aug 17, 2018

ashfurrow commented Aug 17, 2018

jlengstorf commented Aug 17, 2018

jlengstorf commented Aug 18, 2018

orta commented Aug 20, 2018

jlengstorf commented Aug 20, 2018

orta commented Aug 26, 2018 •

edited

Loading

ashfurrow commented Aug 26, 2018

ashfurrow commented Aug 26, 2018

Fix for platformResultsPreMapper/concat problem #355

Fix for platformResultsPreMapper/concat problem #355

Conversation

ashfurrow commented Aug 16, 2018

jlengstorf commented Aug 17, 2018

ashfurrow commented Aug 17, 2018

jlengstorf commented Aug 17, 2018

jlengstorf commented Aug 18, 2018

orta commented Aug 20, 2018

jlengstorf commented Aug 20, 2018

orta commented Aug 26, 2018 • edited Loading

ashfurrow commented Aug 26, 2018

ashfurrow commented Aug 26, 2018

orta commented Aug 26, 2018 •

edited

Loading