Add missing apps methods #733

mandrean · 2017-09-29T02:38:04Z

This pull request provides the missing apps methods and tests.

Fixes #734.

mandrean · 2017-09-29T13:41:24Z

Note: I accidentally pushed an incorrect squash, but fixed it. However, that caused googlebot to remove the cla: yes label. But I have signed the cla. Just FYI.

gmlewis

Thank you, @mandrean!
Just a few minor comments to address, and some advice requested from other reviewers.

gmlewis · 2017-09-29T20:39:59Z

github/apps.go

+// the authenticated GitHub App.
+//
+// GitHub API docs: https://developer.github.com/v3/apps/#get-a-single-github-app
+func (s *AppsService) Get(ctx context.Context, slug string) (*App, *Response, error) {


Can you please change slug to appSlug to match the docs and also add the following comment so that people don't have to follow the link to figure out what an appSlug is?

// Note: appSlug is just the URL-friendly name of your GitHub App. // You can find this on the settings page for your GitHub App // (e.g., https://github.com/settings/apps/:app_slug).

gmlewis · 2017-09-29T20:44:25Z

github/apps.go


 // AppsService provides access to the installation related functions
 // in the GitHub API.
 //
 // GitHub API docs: https://developer.github.com/v3/apps/
 type AppsService service

+// App represents a GitHub App.
+type App struct {
+	ID          *int       `json:"id,omitempty"`


This is actually more of a question for @shurcooL or @willnorris...

In the light of #597, do you think we should start using int64 for numeric IDs for new APIs in order to make the implementation of #597 easier (by providing an example and reducing the number of changes needed)?

Or do you think that we should remain internally consistent and just change everything all at once when #597 is addressed?

IMO it's unclear what the state of #597 is right now. The issue has been opened a while ago, but I haven't seen any reports of issues from production use. I'm not convinced that we should act right away, or how we should act. I think that we need to collect more data in that issue (or, wait until there's a concrete need to act).

Because of that, I think we should not try to incorporate that issue into this one. So, I suggest we stay consistent, and deal with int -> int64 changes (or another solution), if at all, later and separately from this PR.

gmlewis · 2017-09-29T20:49:28Z

github/apps.go

+	// TODO: remove custom Accept header when this API fully launches.
+	req.Header.Set("Accept", mediaTypeIntegrationPreview)
+
+	var i struct {


@shurcooL - what do you think about making this a new type ... struct and include the TotalCount field, then returning a pointer to the struct in the return value of this method?

I'm asking because it seems more consistent with the rest of the package and also allows GitHub to add/tweak their response and we could more flexibly support it however they decide to tweak it.

I agree, I think we should have this method return a pointer to a struct that corresponds to the JSON response in the documentation.

Returning []*Installation would've made sense if the GitHub response was a JSON array of installation objects, but it's a JSON object containing installations field with the array.

The only question is what to name it, if we make it exported (I think we have to, don't we?). Perhaps ListUserInstallationsResponse?

Then again, it's unfortunate it's quite different from the existing ListInstallations method (https://developer.github.com/v3/apps/#find-installations) which does return a JSON array of installations...

I can't help but think GitHub might've made the API inconsistent unintentionally and might want to change it while it's still in preview, so it'd be worth shooting them an email about it.

@gmlewis @shurcooL I did this in my original contribution, but later removed it because I didn't see this pattern being used in other places in go-github where the same type of JSON structure was being handled.

I tried to stay "idiomatic" to your library. But personally I would have mimicked their JSON response as closely as possible.

I didn't see this pattern being used in other places in go-github where the same type of JSON structure was being handled.

Mind sharing examples of that? It would be helpful.

@shurcooL: This is currently in master: https://github.com/google/go-github/blob/master/github/apps_installation.go#L40

But as you can see in the GitHub API docs, the response actually has the total_count field: https://developer.github.com/v3/apps/installations/#list-repositories

So I just reused that same pattern for my ListUserRepos and ListUserInstallations contribution.

Thanks!

@gmlewis What do you think about this finding?

OK, looks like we do this elsewhere... I suppose that if the GitHub API starts expanding these return values to more than just total_count that we can then revisit making a named structure.
So I'm fine with proceeding as written.
Thanks, @mandrean and @shurcooL!

gmlewis · 2017-09-29T20:51:03Z

github/apps_installation.go

+	// TODO: remove custom Accept header when this API fully launches.
+	req.Header.Set("Accept", mediaTypeIntegrationPreview)
+
+	var r struct {


@shurcooL - Same for this one... make a new type ... struct and return it?

gmlewis · 2017-09-29T20:55:03Z

github/apps_installation.go

+	}
+
+	// TODO: remove custom Accept header when this API fully launches.
+	req.Header.Set("Accept", mediaTypeIntegrationPreview)


Is this needed for this endpoint? I didn't see it in the link on line 57.

itay · 2017-10-23T16:11:32Z

Is there any update on this PR in terms of reviewing the updates from @mandrean ? It would be great to have those missing APIs, and it's hard to add them outside the library.

gmlewis

LGTM. Thank you, @mandrean.
Awaiting second LGTM before merging. @shurcooL may not be available currently, so I will request a review from @elliott-beach if he can.

gmlewis · 2017-10-23T16:29:49Z

@mandrean - would you mind resolving the merge conflicts?

dmitshur · 2017-10-30T06:42:38Z

With the help of @itay and PR #769, I'll take care of the merge conflicts here by force pushing the squashed and rebased commit 15ad131 (with merge conflicts resolved) to the PR branch. I've confirmed that it's good and doesn't differ from the contents of this PR (aside from conflict resolution).

Edit: Done.

(For reference, the previous head commit of this PR was 0142ac9.)

dmitshur

I've reviewed at a high-level and didn't spot any major issues. Since this is new functionality, instead of spending an exorbitant amount of time reviewing it for low-level issues prior to merging, I'd prefer we merge it sooner and let people who want to use this API (e.g., @itay) test it out. If any issues or deviations from actual GitHub API are discovered through real-world use, they'll be easy to prioritize and address then. That seems like a more efficient way to move forward.

So, LGTM. It already has @gmlewis's LGTM, so I'll merge. Please let us know if any issues come up.

Thanks @mandrean and @itay (also @gmlewis for reviewing)!

mandrean · 2017-11-02T09:35:06Z

Thanks everyone (and @shurcooL for fixing the merge conflicts - I totally missed @gmlewis question)

❤️

GitHub API docs: https://developer.github.com/v3/apps/.

googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Sep 29, 2017

mandrean force-pushed the feat/add-missing-apps-methods branch from 3d78587 to 7136f17 Compare September 29, 2017 13:29

googlebot added cla: no and removed cla: yes Indication that the PR author has signed a Google Contributor License Agreement. labels Sep 29, 2017

mandrean force-pushed the feat/add-missing-apps-methods branch 2 times, most recently from 73cc038 to 56b2ed1 Compare September 29, 2017 13:37

google deleted a comment from googlebot Sep 29, 2017

googlebot added cla: yes Indication that the PR author has signed a Google Contributor License Agreement. and removed cla: no labels Sep 29, 2017

gmlewis requested changes Sep 29, 2017

View reviewed changes

gmlewis requested a review from dmitshur September 29, 2017 20:57

mandrean force-pushed the feat/add-missing-apps-methods branch from 1580d77 to 0142ac9 Compare October 5, 2017 11:27

gmlewis approved these changes Oct 23, 2017

View reviewed changes

gmlewis requested review from elliott-beach and removed request for dmitshur October 23, 2017 16:29

Add missing apps methods

15ad131

itay mentioned this pull request Oct 29, 2017

Add missing apps methods #769

Closed

dmitshur force-pushed the feat/add-missing-apps-methods branch from 0142ac9 to 15ad131 Compare October 30, 2017 06:43

dmitshur approved these changes Oct 30, 2017

View reviewed changes

dmitshur merged commit 621edea into google:master Oct 30, 2017

dmitshur mentioned this pull request Oct 30, 2017

Split authentication as an app into AppsTransport bradleyfalzon/ghinstallation#7

Merged

mandrean deleted the feat/add-missing-apps-methods branch November 2, 2017 09:42

dmitshur mentioned this pull request Nov 7, 2017

int64 IDs #597

Closed

nbareil pushed a commit to nbareil/go-github that referenced this pull request May 1, 2018

Add missing GitHub Apps methods. (google#733)

b9283ad

GitHub API docs: https://developer.github.com/v3/apps/.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing apps methods #733

Add missing apps methods #733

mandrean commented Sep 29, 2017 •

edited

Loading

mandrean commented Sep 29, 2017

gmlewis left a comment

gmlewis Sep 29, 2017

mandrean Oct 3, 2017

gmlewis Sep 29, 2017

dmitshur Oct 3, 2017

gmlewis Sep 29, 2017

dmitshur Oct 3, 2017

dmitshur Oct 3, 2017

mandrean Oct 3, 2017 •

edited

Loading

dmitshur Oct 3, 2017

mandrean Oct 3, 2017 •

edited

Loading

dmitshur Oct 3, 2017 •

edited

Loading

gmlewis Oct 4, 2017

gmlewis Sep 29, 2017

gmlewis Sep 29, 2017

mandrean Oct 5, 2017

itay commented Oct 23, 2017

gmlewis left a comment

gmlewis commented Oct 23, 2017

dmitshur commented Oct 30, 2017 •

edited

Loading

dmitshur left a comment

mandrean commented Nov 2, 2017

Add missing apps methods #733

Add missing apps methods #733

Conversation

mandrean commented Sep 29, 2017 • edited Loading

mandrean commented Sep 29, 2017

gmlewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandrean Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandrean Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

dmitshur Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itay commented Oct 23, 2017

gmlewis left a comment

Choose a reason for hiding this comment

gmlewis commented Oct 23, 2017

dmitshur commented Oct 30, 2017 • edited Loading

dmitshur left a comment

Choose a reason for hiding this comment

mandrean commented Nov 2, 2017

mandrean commented Sep 29, 2017 •

edited

Loading

mandrean Oct 3, 2017 •

edited

Loading

mandrean Oct 3, 2017 •

edited

Loading

dmitshur Oct 3, 2017 •

edited

Loading

dmitshur commented Oct 30, 2017 •

edited

Loading