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

Support GitHub app authorization #1028

Merged

Conversation

@MorrisLaw
Copy link
Collaborator

commented Oct 6, 2018

Fixes #959 by adding support new webhook event, github_app_authorization.

@googlebot

This comment has been minimized.

Copy link

commented Oct 6, 2018

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 have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@googlebot googlebot added the cla: no label Oct 6, 2018

@MorrisLaw MorrisLaw force-pushed the MorrisLaw:support-github-app-authorization branch from ac30d15 to 63fc1aa Oct 6, 2018

@googlebot

This comment has been minimized.

Copy link

commented Oct 6, 2018

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Oct 6, 2018

@MorrisLaw

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 3, 2018

/retest

@MorrisLaw MorrisLaw force-pushed the MorrisLaw:support-github-app-authorization branch from 55bc6fc to e0c9e81 Nov 21, 2018

@googlebot

This comment has been minimized.

Copy link

commented Nov 21, 2018

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 have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@googlebot googlebot added cla: no and removed cla: yes labels Nov 21, 2018

@MorrisLaw MorrisLaw force-pushed the MorrisLaw:support-github-app-authorization branch from e0c9e81 to 1f47fc6 Nov 21, 2018

@googlebot

This comment has been minimized.

Copy link

commented Nov 21, 2018

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Nov 21, 2018

@MorrisLaw MorrisLaw changed the title [WIP] Support GitHub app authorization Support GitHub app authorization Nov 21, 2018

@MorrisLaw MorrisLaw force-pushed the MorrisLaw:support-github-app-authorization branch from 1f47fc6 to 5c88184 Nov 21, 2018

@MorrisLaw

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 21, 2018

Hey @gmlewis can I get 👀 on this PR

@gmlewis
Copy link
Collaborator

left a comment

Thank you, @MorrisLaw.
This PR has helped me to realize that the code organization as it currently stands is a bit confusing and needs improvement. I've opened up #1055 and will address it soon.

// it will receive the 401 Bad Credentials error.
//
// GitHub API docs: https://developer.github.com/v3/activity/events/types/#githubappauthorizationevent
func (s *AuthorizationsService) RevokedAppAuthorizationEvent(ctx context.Context, user string, opt *ListOptions) ([]*Event, *Response, error) {

This comment has been minimized.

Copy link
@gmlewis

gmlewis Nov 21, 2018

Collaborator

Please move this function and its corresponding test to the authorizations.go and authorizations_test.go files, respectively.

This comment has been minimized.

Copy link
@MorrisLaw

MorrisLaw Nov 23, 2018

Author Collaborator

Thank you @gmlewis for the review. I've addressed your comment with those changes.

@ghost

This comment was marked as spam.

Copy link

commented Nov 23, 2018

@gmlewis
Copy link
Collaborator

left a comment

Thank you, @MorrisLaw - just a couple more tweaks please.

t.Fatalf("Unmarshal Event returned error: %v", err)
}

want := map[string]interface{}{"action": "revoked"}

This comment has been minimized.

Copy link
@gmlewis

gmlewis Nov 23, 2018

Collaborator

Can you please make want be the actual type (&AppAuthorizationEvent{}) and the cast got also to that type before doing a reflect.DeepEqual?

I think that would be much clearer to anyone reading the code what is expected... and if the cast fails, then we have also successfully tested the ParsePayload.

This comment has been minimized.

Copy link
@MorrisLaw

MorrisLaw Nov 27, 2018

Author Collaborator

I'm having trouble writing the code to cast this value because of the value being []*events. Is there something straightforward that I'm missing @gmlewis ? Thank you!

github/event_types.go Outdated Show resolved Hide resolved
github/activity_events.go Outdated Show resolved Hide resolved

@gmlewis gmlewis requested a review from gauntface Nov 23, 2018

github/activity_events.go Outdated Show resolved Hide resolved
// it will receive the 401 Bad Credentials error.
//
// GitHub API docs: https://developer.github.com/v3/activity/events/types/#githubappauthorizationevent
func (s *AuthorizationsService) RevokedAppAuthorizationEvent(ctx context.Context, user string, opt *ListOptions) ([]*Event, *Response, error) {

This comment has been minimized.

Copy link
@gauntface

gauntface Nov 26, 2018

Member

This looks like it's just returned the list of events for the given user which seems different to the function name and description.

I would expect this to be implemented in an events service.

https://developer.github.com/v3/activity/events/

This comment has been minimized.

Copy link
@MorrisLaw

MorrisLaw Nov 27, 2018

Author Collaborator

I initially had it there but @gmlewis reviewed and recommended I move it to authorizations.go.

Another thing to note @gauntface , this is not available in the events API as stated in the description: This event is not available in the Events API.. Despite that fact, do you still think this should be implemented as an events service?

This comment has been minimized.

Copy link
@gauntface

gauntface Nov 28, 2018

Member

I think this function is mixing the events API (https://developer.github.com/v3/activity/events/#list-events-performed-by-a-user) which we would normally have under an events service and the Revoked Auth WebHook event (https://developer.github.com/v3/activity/events/types/#githubappauthorizationevent) which I would expect the golang library to be able to parse the json into a struct.

I believe that is was @gmlewis has done in his PR. But not sure if that is what you are looking for.

@gmlewis

This comment has been minimized.

Copy link
Collaborator

commented Nov 28, 2018

I got really confused by this PR and then finally realized that it is trying to do a bunch more than it really needs to. Additionally, the added function (and the corresponding test) were stomping on an existing GET API endpoint (search the code for users/%v/events and you'll see what I mean).

So I just went ahead and created #1059 to show you what this PR should (in my opinion) look like.

Please let me know if you have any questions.

@MorrisLaw MorrisLaw force-pushed the MorrisLaw:support-github-app-authorization branch from 4967347 to 8f8d252 Nov 29, 2018

@MorrisLaw MorrisLaw force-pushed the MorrisLaw:support-github-app-authorization branch from 8f8d252 to 01175c5 Nov 29, 2018

@MorrisLaw

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 29, 2018

Thank you for doing that. As you found, I apparently did get the existing endpoint mixed up with this PR, so now I see why I was having all those issues. I updated this PR with the changes @gmlewis

@gmlewis
Copy link
Collaborator

left a comment

Thank you, @MorrisLaw!
LGTM.
Awaiting second LGTM before merging to make sure what I suggested is correct.

@gauntface
Copy link
Member

left a comment

Looks great, thank you for sticking with us through all the changes.

@gauntface gauntface merged commit a92f7df into google:master Dec 3, 2018

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MorrisLaw

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 4, 2018

Thanks guys @gauntface @gmlewis ! 😄

@MorrisLaw MorrisLaw deleted the MorrisLaw:support-github-app-authorization branch Dec 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.