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

Add support for apps webhook config endpoints #2096

Merged
merged 5 commits into from Sep 29, 2021

Conversation

rpelliard
Copy link
Contributor

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 in orgs_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 a map[string]interface{} for the Config field, but I think it could be using the HookConfig 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.

@google-cla google-cla bot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Sep 15, 2021
@codecov
Copy link

codecov bot commented Sep 15, 2021

Codecov Report

Merging #2096 (def1026) into master (33ae6f3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head def1026 differs from pull request most recent head e18619d. Consider uploading reports for the commit e18619d to get more accurate results
Impacted file tree graph

@@           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           
Impacted Files Coverage Δ
github/orgs_audit_log.go 100.00% <ø> (ø)
github/apps_hooks.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33ae6f3...e18619d. Read the comment docs.

Copy link
Collaborator

@gmlewis gmlewis left a 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.

github/orgs_audit_log.go Outdated Show resolved Hide resolved
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
Copy link
Collaborator

@gmlewis gmlewis left a 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.

@rpelliard
Copy link
Contributor Author

Hello @gmlewis @wesleimp, do you know when we'd be able to merge this ?

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 29, 2021

Hello @gmlewis @wesleimp, do you know when we'd be able to merge this ?

Maybe @Parker77 has time to take a look at this one.

Copy link

@Parker77 Parker77 left a 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.

github/doc.go Outdated Show resolved Hide resolved
Co-authored-by: Parker77 <Parker77@users.noreply.github.com>
@google-cla
Copy link

google-cla bot commented Sep 29, 2021

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.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes Indication that the PR author has signed a Google Contributor License Agreement. labels Sep 29, 2021
@Parker77
Copy link

@googlebot I fixed it.

@google-cla google-cla bot added cla: yes Indication that the PR author has signed a Google Contributor License Agreement. and removed cla: no labels Sep 29, 2021
@gmlewis
Copy link
Collaborator

gmlewis commented Sep 29, 2021

One minor nit, otherwise LGTM.

Thank you, @Parker77 !
Merging.

@gmlewis gmlewis merged commit 3ad22bc into google:master Sep 29, 2021
@rpelliard rpelliard deleted the apps-webhooks-config branch September 29, 2021 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indication that the PR author has signed a Google Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants