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

Potential problem with github.utils.fileContents #245

Closed
mxstbr opened this issue Apr 25, 2018 · 20 comments
Closed

Potential problem with github.utils.fileContents #245

mxstbr opened this issue Apr 25, 2018 · 20 comments
Labels

Comments

@mxstbr
Copy link
Member

mxstbr commented Apr 25, 2018

I tried adding my very own danger-plugin-flow to our Peril setup (https://github.com/withspectrum/peril-settings), but as soon as I did that and added an unflowtyped file (to make the plugin fail to see whether it works or not) I got a "Bad credentials" error:

screen shot 2018-04-25 at 21 02 11

Any clues what could be happening here? I specifically used github.utils.fileContents instead of fs for Peril compat, but I might totally be missing something.

@mxstbr
Copy link
Member Author

mxstbr commented Apr 25, 2018

It's weird because Peril managed to comment in the PR, so it obviously got the GitHub token... somewhere, but then somewhere else it doesn't anymore or something?

@orta
Copy link
Member

orta commented Apr 25, 2018

github.utils.fileContents doesn't use the same Peril API under the hood as the main comment posting, but is a transformed version of the OctoKit module - https://github.com/danger/danger-js/blob/c4a1d99fb7bdc387cd7876326ec93e09baf3a942/source/platforms/github/GitHubUtils.ts#L32-L51

It's possible that the GitHub API given to the create that fileContents isn't author correctly

@mxstbr
Copy link
Member Author

mxstbr commented Apr 25, 2018

@mxstbr
Copy link
Member Author

mxstbr commented Apr 25, 2018

Too many layers of indirection for an outside to follow 😅 Not sure where the problem could lie beyond that method, any ideas?

@orta
Copy link
Member

orta commented Apr 25, 2018

Yeah, no, I woundn't recommend digging into this one! The heart of the danger runner is super complex I'm afraid

But it's something around this.

@orta
Copy link
Member

orta commented Apr 25, 2018

I have a long flight tomorrow where I intend to work on #240 - so I'll keep my eyes open for this too

@mxstbr
Copy link
Member Author

mxstbr commented Apr 25, 2018

Sounds great to me, thank you!

@orta
Copy link
Member

orta commented Apr 27, 2018

This should be fixed in #247

@mxstbr
Copy link
Member Author

mxstbr commented Apr 27, 2018

Let me know when a new release is out—I'll have to deploy a fresh version of Peril for it to update, right?

@orta
Copy link
Member

orta commented Apr 27, 2018

@mxstbr
Copy link
Member Author

mxstbr commented Apr 27, 2018

On it—will update, then reenable and see if it's fixed!

@mxstbr
Copy link
Member Author

mxstbr commented Apr 27, 2018

IT WORKED!!! 🎉

screen shot 2018-04-27 at 16 25 02

Thanks for the quick resolution here, much appreciated.

@mxstbr mxstbr closed this as completed Apr 27, 2018
@orta
Copy link
Member

orta commented Apr 27, 2018

Thanks for the solid bug report, I should move some of my plugins to use that API too

@mxstbr
Copy link
Member Author

mxstbr commented Apr 27, 2018

Ah crap, I spoke too soon, now it's no longer editing its own comments:

screen shot 2018-04-27 at 16 25 48

@mxstbr mxstbr reopened this Apr 27, 2018
@mxstbr
Copy link
Member Author

mxstbr commented Apr 27, 2018

I definitely have all the env vars set correctly since this worked before as long as I wasn't using the GitHub API, so it's gotta be something in that same area of the code probably? Any quick gut ideas?

@orta
Copy link
Member

orta commented Apr 27, 2018

Hrm, could it be #219 ?

@orta
Copy link
Member

orta commented Apr 27, 2018

However, the env var to double check is PERIL_BOT_USER_ID

@mxstbr
Copy link
Member Author

mxstbr commented Apr 27, 2018

Definitely sounds like #219

@orta
Copy link
Member

orta commented Apr 27, 2018

OK, I'll have a proper think about this over the weekend.

It should be possible to add support coalescing multiple runs to all happen sycnronously without a db, devoted some time to the idea on the airplane

@mxstbr
Copy link
Member Author

mxstbr commented Apr 27, 2018

Closing as a duplicate of #219 and tracking that!

@mxstbr mxstbr closed this as completed Apr 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants