Enhancement/add remove repository #749

MorrisLaw · 2017-10-11T17:57:24Z

Added support for preview installations API for the following:

adding a repository to an installation
removing a repository from an installation

…ove-repository

…emove-repository

…lock.

…m/MorrisLaw/go-github into enhancement/add-remove-repository

dmitshur · 2017-10-12T20:25:40Z

I've attempted to tackle this issue but my changes have failed in travis checks. Can someone please point me in the right direction, the changes are located in PR #749. I am new to Go and this github client in general, so I may be missing something obvious. Thank you everyone for your patience and advice! (: @shurcooL @gmlewis

@MorrisLaw, you can click on the icon here to see the CI results:

It links to the CI page. There, you can see all 4 jobs failed. Pick the first one (Go 1.9.x job), and look at the log. If you scan towards the bottom, you'll see something in red, that's the first failure:

$ go test -v -race ./...
# github.com/google/go-github/github
import cycle not allowed in test
package github.com/google/go-github/github (test)
	imports github.com/google/go-github/github
FAIL	github.com/google/go-github/github [setup failed]
?   	github.com/google/go-github/example/appengine	[no test files]
?   	github.com/google/go-github/example/basicauth	[no test files]
?   	github.com/google/go-github/test/fields	[no test files]
?   	github.com/google/go-github/test/integration	[no test files]
The command "go test -v -race ./..." exited with 1.

The command that failed was go test -v -race ./.... You will need to fix that issue.

dmitshur · 2017-10-12T20:28:56Z

github/apps_installation_test.go

@@ -11,6 +11,8 @@ import (
 	"net/http"
 	"reflect"
 	"testing"
+
+	"github.com/google/go-github/github"


This is an internal package test .go file. Therefore, you don't need to import that package - you're already in it.

Instead of writing github.ErrorResponse below, you can access it with just ErrorResponse.

That will fix the problem.

You can read more about internal/external tests at https://golang.org/cmd/go/#hdr-Test_packages.

…rResponse.

…Repo test.

googlebot · 2017-10-13T02:29:43Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). 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.

dmitshur · 2017-10-13T03:08:48Z

github/apps_installation.go

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


mediaTypeV3 is already the default set in NewRequest method:

go-github/github/github.go

Line 269 in 0f6d3ce

req.Header.Set("Accept", mediaTypeV3)

So no need to do this, you can delete the two lines above.

dmitshur · 2017-10-13T03:13:14Z

github/apps_installation.go

+// AddRepo adds a single repository to an installation.
+//
+// GitHub API docs: https://developer.github.com/v3/apps/installations/#add-repository-to-installation
+func (s *AppsService) AddRepo(ctx context.Context, instID, repoID int) (*Repository, *Response, error) {


I think we should call this method AddRepository rather than AddRepo. We already use the full "Repository" word in many other method names, e.g.:

ActivityService.GetRepositorySubscription

ActivityService.DeleteRepositorySubscription

IssuesService.ListRepositoryEvents

dmitshur · 2017-10-13T03:13:43Z

github/apps_installation.go

+// RemoveRepo removes a single repository from an installation.
+//
+// GitHub docs: https://developer.github.com/v3/apps/installations/#remove-repository-from-installation
+func (s *AppsService) RemoveRepo(ctx context.Context, instID, repoID int) (*Response, error) {


Similarly, this would be RemoveRepository.

dmitshur · 2017-10-13T03:15:02Z

github/apps_installation.go

+	r := new(Repository)
+	resp, err := s.client.Do(ctx, req, r)
+	if err != nil {
+		return nil, nil, err


Even if err != nil, you should return resp rather than nil *Response:

resp, err := s.client.Do(ctx, req, r) if err != nil { return nil, resp, err }

This is what all other methods do.

googlebot · 2017-10-13T03:20:53Z

CLAs look good, thanks!

dmitshur

Really minor remaining things.

dmitshur · 2017-10-13T03:44:42Z

github/apps_installation_test.go

+
+	repo, _, err := client.Apps.AddRepository(context.Background(), 1, 1)
+	if err != nil {
+		t.Errorf("Apps.AddRepo returned error: %v", err)


It'd be nice to rename the method name here too.

dmitshur · 2017-10-13T03:44:47Z

github/apps_installation_test.go

+
+	want := &Repository{ID: Int(1), Name: String("n"), Description: String("d"), Owner: &User{Login: String("l")}, License: &License{Key: String("mit")}}
+	if !reflect.DeepEqual(repo, want) {
+		t.Errorf("AddRepo returned %+v, want %+v", repo, want)


dmitshur · 2017-10-13T03:44:52Z

github/apps_installation_test.go

+
+	_, err := client.Apps.RemoveRepository(context.Background(), 1, 1)
+	if err != nil {
+		t.Errorf("Apps.RemoveRepo returned error: %v", err)


As well as here.

…tion names.

dmitshur

I've spotted a small but critical issue we need to fix.

After that, I don't have any other suggestions and it'll have my LGTM. Thanks @MorrisLaw!

dmitshur · 2017-10-13T17:57:10Z

github/apps_installation.go

+//
+// GitHub API docs: https://developer.github.com/v3/apps/installations/#add-repository-to-installation
+func (s *AppsService) AddRepository(ctx context.Context, instID, repoID int) (*Repository, *Response, error) {
+	u := fmt.Sprintf("/apps/installations/%v/repositories/%v", instID, repoID)


We need to not include the starting slash in the URLs, otherwise enterprise GH URLs will not work.

This should be:

u := fmt.Sprintf("apps/installations/%v/repositories/%v", instID, repoID)

dmitshur · 2017-10-13T17:58:53Z

github/apps_installation.go

+//
+// GitHub docs: https://developer.github.com/v3/apps/installations/#remove-repository-from-installation
+func (s *AppsService) RemoveRepository(ctx context.Context, instID, repoID int) (*Response, error) {
+	u := fmt.Sprintf("/apps/installations/%v/repositories/%v", instID, repoID)


Same here, please remove the first slash in the URL. You'll notice all other endpoints don't include it either.

MorrisLaw · 2017-10-13T18:16:50Z

Thank you for the thorough reviewing and for providing all of the useful feedback! @shurcooL

dmitshur

LGTM. I'm not spotting any other issues.

Thank you @MorrisLaw!

I'll wait for another reviewer, and we can merge after that.

gmlewis

LGTM.
Thank you, @MorrisLaw and @shurcooL!

GitHub API docs: - https://developer.github.com/v3/apps/installations/#add-repository-to-installation - https://developer.github.com/v3/apps/installations/#remove-repository-from-installation

MorrisLaw and others added 30 commits August 21, 2017 22:25

Added my name to AUTHORS list.

aa10fb4

Added API route for adding a repo to an installation.

7772ba0

Added api route for removing repo from installation.

5d99cdf

Moved Add/Remove Repository functions to app_installation.

2eab2b3

Simplified logic in remove repository.

a35a566

Style fix.

3e14143

Refactored function names.

44e0f51

Renamed functions back to having Repository suffix.

defe8b8

Change argument from Installation to Repository.

f31595a

Changed variable name for Repository.

5934515

Shortened function name.

e20f019

Added tests for Add and Remove repo.

16bcc33

Fixed golint issues.

f90fa0d

Merge remote-tracking branch 'origin/master' into enhancement/add-rem…

71f1694

…ove-repository

Removed merge conflict from adding my name to list.

585efab

Ran go fmt on file.

030ae55

Ran go fmt on file.

11aefc3

Changed s *AppService to s *AppsService .

173da1c

Added import and removed extra value from return in removeRepo.

2c3db48

Fixed some issues discovered by go test .

158fea8

Merge branch 'master' into enhancement/add-remove-repository

0c7acc2

Merge remote-tracking branch 'upstream/master' into enhancement/add-r…

9b8d40c

…emove-repository

Added colons to url path for add and remove repo.

b9c5873

Removed colons from http put/delete paths.

e132615

Debugging broken test and using example from repos_test as building b…

926a90c

…lock.

Merge branch 'master' into enhancement/add-remove-repository

f1bf61b

Merge branch 'enhancement/add-remove-repository' of https://github.co…

5e8d98c

…m/MorrisLaw/go-github into enhancement/add-remove-repository

Refactored url string.

a612b5e

Add repo test passes.

f35c3c9

Uncommented custom accept header.

d6c991e

Ran go fmt on codebase.

60af6e6

MorrisLaw mentioned this pull request Oct 11, 2017

Support preview Installations API - updated routes for adding or removing a repository #666

Closed

dmitshur reviewed Oct 12, 2017

View reviewed changes

Jeremy Morris added 2 commits October 12, 2017 21:57

Removed unnecessary import and changed *github.ErrorResponse to *Erro…

8e876e4

…rResponse.

Fixed url path for RemoveRepo and removed extra conditional in Remove…

42faa4d

…Repo test.

googlebot added the cla: no label Oct 13, 2017

MorrisLaw added 2 commits October 12, 2017 22:51

Added newline at eof, to restart build after CLA email fix.

326457b

Removed newline from test file.

b4b56c3

dmitshur reviewed Oct 13, 2017

View reviewed changes

Fixed suggestions for naming functions, and fixing error check.

432dab7

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

dmitshur reviewed Oct 13, 2017

View reviewed changes

Jeremy Morris added 2 commits October 13, 2017 00:09

Added full function names to Errorf strings, to match referenced func…

c01f313

…tion names.

Removed mediatypev3.

41d02d7

dmitshur requested changes Oct 13, 2017

View reviewed changes

dmitshur requested review from gmlewis and elliott-beach October 13, 2017 18:01

Removed starting slash in URLs.

b5f3a3b

dmitshur approved these changes Oct 13, 2017

View reviewed changes

dmitshur mentioned this pull request Oct 13, 2017

Make tests catch invalid absolute URLs in methods. #752

Closed

gmlewis approved these changes Oct 13, 2017

View reviewed changes

dmitshur merged commit 99750d1 into google:master Oct 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement/add remove repository #749

Enhancement/add remove repository #749

MorrisLaw commented Oct 11, 2017

dmitshur commented Oct 12, 2017

dmitshur Oct 12, 2017

googlebot commented Oct 13, 2017

dmitshur Oct 13, 2017 •

edited

Loading

dmitshur Oct 13, 2017

dmitshur Oct 13, 2017

dmitshur Oct 13, 2017 •

edited

Loading

googlebot commented Oct 13, 2017

dmitshur left a comment

dmitshur Oct 13, 2017

dmitshur Oct 13, 2017

dmitshur Oct 13, 2017

dmitshur left a comment

dmitshur Oct 13, 2017

dmitshur Oct 13, 2017

MorrisLaw commented Oct 13, 2017

dmitshur left a comment

gmlewis left a comment

Enhancement/add remove repository #749

Enhancement/add remove repository #749

Conversation

MorrisLaw commented Oct 11, 2017

dmitshur commented Oct 12, 2017

Choose a reason for hiding this comment

googlebot commented Oct 13, 2017

dmitshur Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

googlebot commented Oct 13, 2017

dmitshur 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

dmitshur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MorrisLaw commented Oct 13, 2017

dmitshur left a comment

Choose a reason for hiding this comment

gmlewis left a comment

Choose a reason for hiding this comment

dmitshur Oct 13, 2017 •

edited

Loading

dmitshur Oct 13, 2017 •

edited

Loading