-
Notifications
You must be signed in to change notification settings - Fork 85
add implementation of permissions inputs #217
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
add implementation of permissions inputs #217
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can merge this one in already, and continue next week investigating the double requests in the snapshots
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good to go.
main( | ||
const permissions = getPermissionsFromInputs(process.env); | ||
|
||
export default main( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are exporting the promise returned by main()
for testing. We need to await the execution in order to snapshot all requests that were sent by it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much, @gr2m! ✨ I left a few minor suggestions for your consideration.
Co-authored-by: Parker Brown <[email protected]>
Co-authored-by: Parker Brown <[email protected]>
Co-authored-by: Parker Brown <[email protected]>
let's ship it 👍🏼 What's the benefit of using
I have never seen that used before |
It adds syntax highlighting. You can use No syntax highlighting:
Syntax highlighting with > echo $FOOBAR
Hello World! Syntax highlighting with > echo $FOOBAR
Hello World! |
This is ready for review now.
Our tests now log out each request, including request body for the
POST
request to create an installation access token. That way it becomes part of our snapshot and we can verify that the correct requests are being sent.I don't like the way I implemented the logging, it's manually on every
.intercept()
call. And I had to use a hack with aonce()
helper method because thebody()
andpath()
callbacks get invoked twice by undici for some reason. Ideally there would be a way to add a "request" listener and just log that out, I'll ask the undici folks if there is a way, I couldn't find any.Anyway, this is now good for merging