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 support for apps webhook config endpoints #2096
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2096 +/- ##
=======================================
Coverage 97.77% 97.78%
=======================================
Files 109 110 +1
Lines 9724 9746 +22
=======================================
+ Hits 9508 9530 +22
Misses 150 150
Partials 66 66
Continue to review full report at Codecov.
|
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.
Thank you, @rpelliard !
One tiny nit to fix, please, but otherwise LGTM.
Awaiting second LGTM before merging.
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
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.
LGTM.
Awaiting second LGTM before merging.
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.
One minor nit, otherwise LGTM.
Co-authored-by: Parker77 <Parker77@users.noreply.github.com>
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
@googlebot I fixed it. |
Thank you, @Parker77 ! |
This adds support for two Apps API endpoints (see #2095)
Both endpoints require authentication as an app (with a JWT), which can be dealt with by the underlying transport.
A few notes:
I reused / extended the
HookConfig
struct which was defined inorgs_audit_logs.go
, since it represents the same object.The
InsecureSSL
field of the struct is a*string
. However, GitHub only returns"0"
or"1"
(insecure SSL disabled / enabled), so I think it'd be more user-friendly to use a*bool
, and write some logic so that boolean values are correctly (un)marshaled from"0"
/"1"
. I'd be happy to write the code to deal with this, but that would be a breaking change so I wanted to check with you first.This is probably out of scope of this PR, but the
Hook
struct used for repository / organization webhooks endpoints is using amap[string]interface{}
for theConfig
field, but I think it could be using theHookConfig
struct as well since it represents the same object. Happy to make that change in a separate PR, but this would be a breaking change as well.