Update apps.go #1238

emoryruscus · 2019-07-23T15:54:38Z

Add installation token parameters, allowing creation of permission-scoped github tokens.

https://developer.github.com/v3/apps/#create-a-new-installation-token

Add installation token parameters, allowing creation of permission-scoped github tokens.

googlebot · 2019-07-23T15:54:44Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

Add test for creating installation tokens with additional parameters. https://developer.github.com/v3/apps/#create-a-new-installation-token

googlebot · 2019-07-23T20:37:22Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

gmlewis

Thanks, @emoryruscus.
Please address the comments and then we will go for another round of review.

gmlewis · 2019-07-23T23:38:25Z

github/apps.go

+// InstallationTokenParameters allow restricting a token's access to specific repositories.
+type InstallationTokenParameters struct {
+	// The ids of the repositories that the installation token can access.
+	// Providing repository ids restricts the access of an installation token to specific repositories.


Let's please change ids in the comment to IDs for the auto-generated godocs.

gmlewis · 2019-07-23T23:40:30Z

github/apps.go

@@ -194,10 +211,10 @@ func (s *AppsService) ListUserInstallations(ctx context.Context, opt *ListOption
 // 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 int64) (*InstallationToken, *Response, error) {
+func (s *AppsService) CreateInstallationToken(ctx context.Context, id int64, body ...interface{}) (*InstallationToken, *Response, error) {


Please let's not use ...interface{} but keep this repo strictly typed.
There are many other examples where we pass in a pointer to an optional struct and we usually call it opt, as in:
opt *InstallationTokenOptions

gmlewis · 2019-07-23T23:40:52Z

github/apps.go

+}
+
+// InstallationTokenParameters allow restricting a token's access to specific repositories.
+type InstallationTokenParameters struct {


Let's please rename this to InstallationTokenOptions to be more consistent with the rest of the repo.

gmlewis · 2019-07-23T23:41:38Z

github/apps.go

+	Token        *string                  `json:"token,omitempty"`
+	ExpiresAt    *time.Time               `json:"expires_at,omitempty"`
+	Permissions  *InstallationPermissions `json:"permissions,omitempty"`
+	Repositories *[]Repository            `json:"repositories,emitempty"`


Please change *[]Repository to []*Repository instead.

gmlewis · 2019-07-23T23:42:43Z

github/apps.go

@@ -282,3 +299,32 @@ func (s *AppsService) getInstallation(ctx context.Context, url string) (*Install

 	return i, resp, nil
 }
+
+// GetReadWriter converts a body interface into an io.ReadWriter object.
+func GetReadWriter(body interface{}) (io.ReadWriter, error) {


I don't think this or the next function are necessary. Please remove them.

They are used in apps_test.go, and I would like to use them in the ghinstallation package in a subsequent pull request as well. It's useful for comparing results in testing, and will be useful in converting installation token options into a ReadWriter to pass to http.NewRequest in ghinstallation.

https://github.com/bradleyfalzon/ghinstallation

shanempope · 2019-07-24T01:08:07Z

FYI This PR has related / same feature: #1182

gmlewis · 2019-07-24T01:50:35Z

Ah, thank you for the pointer, @shanempope!

It appears that #1182 never got completed... so when this PR is merged we can close that other PR, unless that one gets finished first, then we can close this one. 😃

Change 'parameters' to 'options'.

emoryruscus · 2019-07-30T15:15:07Z

Friendly ping, @gmlewis do you have a moment to look over the small changes I made?

gmlewis · 2019-07-30T15:23:31Z

Hi @emoryruscus, I was waiting to review again until after you fix the broken Travis CI build.

Also, I'm not convinced that we need the helper functions and do not wish to increase the maintenance burden of this repo by adding unnecessary functions... I haven't read your argument yet, just saw that you added one. I don't have time now, but was going to give it more detailed attention once you fix the broken build.

Add GetPermissions() and GetRepositories() accessor functions.

Try to comply with Travis build failure.

Reformat file for Travis.

gofmt fix spacing issue

codecov · 2019-08-05T15:48:18Z

Codecov Report

Merging #1238 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1238      +/-   ##
==========================================
+ Coverage   73.36%   73.42%   +0.06%     
==========================================
  Files          86       86              
  Lines        6026     6040      +14     
==========================================
+ Hits         4421     4435      +14     
  Misses        836      836              
  Partials      769      769

Impacted Files	Coverage Δ
github/apps.go	`67.81% <100%> (ø)`	⬆️
github/github.go	`89.31% <0%> (ø)`	⬆️
github/repos.go	`75.16% <0%> (+0.79%)`	⬆️

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 c756c32...9d6fd70. Read the comment docs.

Move GetReadWriter and GetReadCloser from apps.go to apps_test.go to improve coverage.

Fix imports.

author Emory Ruscus <[email protected]> 1563895211 -0400 committer Emory Ruscus <[email protected]> 1565621142 -0400 Update apps.go Add installation token parameters, allowing creation of permission-scoped github tokens. Update apps_test.go Add test for creating installation tokens with additional parameters. https://developer.github.com/v3/apps/#create-a-new-installation-token Update apps.go - Change 'parameters' to 'options' Update apps_test.go Change 'parameters' to 'options'. Update apps_test.go Update github-accessors.go Add GetPermissions() and GetRepositories() accessor functions. Update github-accessors.go Try to comply with Travis build failure. Update github-accessors.go Reformat file for Travis. gofmt spacing fix Move reader functions for coverage test. Move GetReadWriter and GetReadCloser from apps.go to apps_test.go to improve coverage. Move reader functions for coverage test. Move GetReadWriter and GetReadCloser from apps.go to apps_test.go to improve coverage. Update apps.go Fix imports. Update apps_test.go Fix imports.

…to patch-1 Tip of current branch is behind its remote counterpart.

emoryruscus · 2019-08-12T15:11:14Z

Sorry about that! Fixed the broken Travis CI build. Also moved the helper functions into apps_test.go, which I think you will like more.

gmlewis

Thank you, @emoryruscus!

This looks much better to me now.
Just one minor tweak, please, and then I think we'll be ready to merge once we get a second LGTM.

gmlewis · 2019-08-13T00:47:32Z

github/apps.go

+	Token        *string                  `json:"token,omitempty"`
+	ExpiresAt    *time.Time               `json:"expires_at,omitempty"`
+	Permissions  *InstallationPermissions `json:"permissions,omitempty"`
+	Repositories []*Repository            `json:"repositories,emitempty"`


This should be omitempty, not emitempty.

Great catch, thanks! Done.

gmlewis · 2019-08-13T00:57:45Z

github/apps_test.go

@@ -330,3 +382,31 @@ func TestAppsService_FindUserInstallation(t *testing.T) {
 		t.Errorf("Apps.FindUserInstallation returned %+v, want %+v", installation, want)
 	}
 }
+
+// GetReadWriter converts a body interface into an io.ReadWriter object.


Yes, I much prefer these two new helper functions be used for testing only... thank you!

In fact, since they are rather general purpose, I would be fine to move them to github_test.go if you want, or we could just leave them here for now and move them in a later PR if they become more generally used.

We'll see what @gauntface thinks about them.

Welcome! Will leave them here for now if that is alright.

Change 'emitempty' to 'omitempty'.

gmlewis

Thank you, @emoryruscus!

LGTM.

Awaiting second LGTM before merging.

emoryruscus · 2019-08-19T17:46:11Z

No problem! @gauntface is there anything else you would like to see in this change?

gauntface

LGTM from me =. I'd keep the helpers where they are until someone wants to re-use them.

emoryruscus · 2019-08-23T19:58:50Z

Sounds good, thanks!

gmlewis · 2019-08-24T01:23:38Z

Thank you, @emoryruscus and @gauntface !

Merging.

Note to self: This is a breaking API change so the major version needs to be bumped after merging.

Closes google#1182.

Update apps.go

b91a2a6

Add installation token parameters, allowing creation of permission-scoped github tokens.

googlebot added the cla: no label Jul 23, 2019

Update apps_test.go

d0834e0

Add test for creating installation tokens with additional parameters. https://developer.github.com/v3/apps/#create-a-new-installation-token

googlebot added cla: yes Indication that the PR author has signed a Google Contributor License Agreement. and removed cla: no labels Jul 23, 2019

gmlewis requested changes Jul 23, 2019

View reviewed changes

emoryruscus added 2 commits July 24, 2019 11:14

Update apps.go - Change 'parameters' to 'options'

0d94311

Update apps_test.go

ad7cafc

Change 'parameters' to 'options'.

kimtore mentioned this pull request Jul 30, 2019

Add Support For Installation Token Parameters #1237

Closed

emoryruscus and others added 7 commits August 1, 2019 15:26

Update apps_test.go

243b6b0

Update github-accessors.go

89c7ea2

Add GetPermissions() and GetRepositories() accessor functions.

Update github-accessors.go

431e8e6

Try to comply with Travis build failure.

Update github-accessors.go

bc6f117

Reformat file for Travis.

gofmt spacing fix

96f8146

Update apps.go

d00e1b1

gofmt fix spacing issue

gofmt spacing fix2

040e921

emoryruscus and others added 6 commits August 8, 2019 11:05

Move reader functions for coverage test.

24ee2e6

Move GetReadWriter and GetReadCloser from apps.go to apps_test.go to improve coverage.

Move reader functions for coverage test.

d069332

Move GetReadWriter and GetReadCloser from apps.go to apps_test.go to improve coverage.

Update apps.go

9a57f54

Fix imports.

Update apps_test.go

14c5f86

Fix imports.

Merge branch 'patch-1' of https://github.com/emoryruscus/go-github in…

5033cff

…to patch-1 Tip of current branch is behind its remote counterpart.

gmlewis requested changes Aug 13, 2019

View reviewed changes

Update apps.go

9d6fd70

Change 'emitempty' to 'omitempty'.

gmlewis approved these changes Aug 13, 2019

View reviewed changes

gmlewis requested a review from gauntface August 13, 2019 14:26

gauntface approved these changes Aug 23, 2019

View reviewed changes

gmlewis merged commit 61d9e5b into google:master Aug 24, 2019

gmlewis added a commit that referenced this pull request Aug 24, 2019

Bump major version to v28 due to #1238

2245c71

emoryruscus deleted the patch-1 branch August 24, 2019 17:10

emoryruscus mentioned this pull request Aug 26, 2019

Add repository IDs and permissions to refreshToken parameters. bradleyfalzon/ghinstallation#24

Merged

n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020

Update apps.go (google#1238)

39654d3

Closes google#1182.

n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020

Bump major version to v28 due to google#1238

28cd210

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update apps.go #1238

Update apps.go #1238

emoryruscus commented Jul 23, 2019

googlebot commented Jul 23, 2019

googlebot commented Jul 23, 2019

gmlewis left a comment

gmlewis Jul 23, 2019

emoryruscus Jul 24, 2019

gmlewis Jul 23, 2019

emoryruscus Jul 24, 2019

gmlewis Jul 23, 2019

emoryruscus Jul 24, 2019

gmlewis Jul 23, 2019

emoryruscus Jul 24, 2019

gmlewis Jul 23, 2019

emoryruscus Jul 24, 2019

shanempope commented Jul 24, 2019

gmlewis commented Jul 24, 2019

emoryruscus commented Jul 30, 2019

gmlewis commented Jul 30, 2019

codecov bot commented Aug 5, 2019 •

edited

Loading

emoryruscus commented Aug 12, 2019

gmlewis left a comment

gmlewis Aug 13, 2019

emoryruscus Aug 13, 2019

gmlewis Aug 13, 2019

emoryruscus Aug 13, 2019

gmlewis left a comment

emoryruscus commented Aug 19, 2019

gauntface left a comment

emoryruscus commented Aug 23, 2019

gmlewis commented Aug 24, 2019

Update apps.go #1238

Update apps.go #1238

Conversation

emoryruscus commented Jul 23, 2019

googlebot commented Jul 23, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented Jul 23, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shanempope commented Jul 24, 2019

gmlewis commented Jul 24, 2019

emoryruscus commented Jul 30, 2019

gmlewis commented Jul 30, 2019

codecov bot commented Aug 5, 2019 • edited Loading

Codecov Report

emoryruscus commented Aug 12, 2019

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

gmlewis left a comment

Choose a reason for hiding this comment

emoryruscus commented Aug 19, 2019

gauntface left a comment

Choose a reason for hiding this comment

emoryruscus commented Aug 23, 2019

gmlewis commented Aug 24, 2019

codecov bot commented Aug 5, 2019 •

edited

Loading