-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add missing apps methods #733
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,14 +5,69 @@ | |
|
||
package github | ||
|
||
import "context" | ||
import ( | ||
"context" | ||
"fmt" | ||
"time" | ||
) | ||
|
||
// 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"` | ||
Owner *User `json:"owner,omitempty"` | ||
Name *string `json:"name,omitempty"` | ||
Description *string `json:"description,omitempty"` | ||
ExternalURL *string `json:"external_url,omitempty"` | ||
HTMLURL *string `json:"html_url,omitempty"` | ||
CreatedAt *time.Time `json:"created_at,omitempty"` | ||
UpdatedAt *time.Time `json:"updated_at,omitempty"` | ||
} | ||
|
||
// InstallationToken represents an installation token. | ||
type InstallationToken struct { | ||
Token *string `json:"token,omitempty"` | ||
ExpiresAt *time.Time `json:"expires_at,omitempty"` | ||
} | ||
|
||
// Get a single GitHub App. Passing the empty string will get | ||
// the authenticated GitHub App. | ||
// | ||
// 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). | ||
// | ||
// GitHub API docs: https://developer.github.com/v3/apps/#get-a-single-github-app | ||
func (s *AppsService) Get(ctx context.Context, appSlug string) (*App, *Response, error) { | ||
var u string | ||
if appSlug != "" { | ||
u = fmt.Sprintf("apps/%v", appSlug) | ||
} else { | ||
u = "app" | ||
} | ||
|
||
req, err := s.client.NewRequest("GET", u, nil) | ||
if err != nil { | ||
return nil, nil, err | ||
} | ||
|
||
// TODO: remove custom Accept header when this API fully launches. | ||
req.Header.Set("Accept", mediaTypeIntegrationPreview) | ||
|
||
app := new(App) | ||
resp, err := s.client.Do(ctx, req, app) | ||
if err != nil { | ||
return nil, resp, err | ||
} | ||
|
||
return app, resp, nil | ||
} | ||
|
||
// ListInstallations lists the installations that the current GitHub App has. | ||
// | ||
// GitHub API docs: https://developer.github.com/v3/apps/#find-installations | ||
|
@@ -38,3 +93,77 @@ func (s *AppsService) ListInstallations(ctx context.Context, opt *ListOptions) ( | |
|
||
return i, resp, nil | ||
} | ||
|
||
// GetInstallation returns the specified installation. | ||
// | ||
// GitHub API docs: https://developer.github.com/v3/apps/#get-a-single-installation | ||
func (s *AppsService) GetInstallation(ctx context.Context, id int) (*Installation, *Response, error) { | ||
u := fmt.Sprintf("app/installations/%v", id) | ||
|
||
req, err := s.client.NewRequest("GET", u, nil) | ||
if err != nil { | ||
return nil, nil, err | ||
} | ||
|
||
// TODO: remove custom Accept header when this API fully launches. | ||
req.Header.Set("Accept", mediaTypeIntegrationPreview) | ||
|
||
i := new(Installation) | ||
resp, err := s.client.Do(ctx, req, i) | ||
if err != nil { | ||
return nil, resp, err | ||
} | ||
|
||
return i, resp, nil | ||
} | ||
|
||
// ListUserInstallations lists installations that are accessible to the authenticated user. | ||
// | ||
// GitHub API docs: https://developer.github.com/v3/apps/#list-installations-for-user | ||
func (s *AppsService) ListUserInstallations(ctx context.Context, opt *ListOptions) ([]*Installation, *Response, error) { | ||
u, err := addOptions("user/installations", opt) | ||
if err != nil { | ||
return nil, nil, err | ||
} | ||
|
||
req, err := s.client.NewRequest("GET", u, nil) | ||
if err != nil { | ||
return nil, nil, err | ||
} | ||
|
||
// TODO: remove custom Accept header when this API fully launches. | ||
req.Header.Set("Accept", mediaTypeIntegrationPreview) | ||
|
||
var i struct { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shurcooL - what do you think about making this a new 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 The only question is what to name it, if we make it exported (I think we have to, don't we?). Perhaps There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then again, it's unfortunate it's quite different from the existing 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 I tried to stay "idiomatic" to your library. But personally I would have mimicked their JSON response as closely as possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Mind sharing examples of that? It would be helpful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shurcooL: This is currently in But as you can see in the GitHub API docs, the response actually has the So I just reused that same pattern for my There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! @gmlewis What do you think about this finding? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
Installations []*Installation `json:"installations"` | ||
} | ||
resp, err := s.client.Do(ctx, req, &i) | ||
if err != nil { | ||
return nil, resp, err | ||
} | ||
|
||
return i.Installations, resp, nil | ||
} | ||
|
||
// CreateInstallationToken creates a new installation token. | ||
// | ||
// GitHub API docs: https://developer.github.com/v3/apps/#create-a-new-installation-token | ||
func (s *AppsService) CreateInstallationToken(ctx context.Context, id int) (*InstallationToken, *Response, error) { | ||
u := fmt.Sprintf("installations/%v/access_tokens", id) | ||
|
||
req, err := s.client.NewRequest("POST", u, nil) | ||
if err != nil { | ||
return nil, nil, err | ||
} | ||
|
||
// TODO: remove custom Accept header when this API fully launches. | ||
req.Header.Set("Accept", mediaTypeIntegrationPreview) | ||
|
||
t := new(InstallationToken) | ||
resp, err := s.client.Do(ctx, req, t) | ||
if err != nil { | ||
return nil, resp, err | ||
} | ||
|
||
return t, resp, nil | ||
} |
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 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?
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.
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.