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

Enhancement/add remove repository #749

Merged
merged 41 commits into from Oct 13, 2017

Conversation

@MorrisLaw
Copy link
Collaborator

commented Oct 11, 2017

Added support for preview installations API for the following:

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

MorrisLaw and others added some commits Aug 22, 2017

Jeremy Morris
Jeremy Morris
@dmitshur

This comment has been minimized.

Copy link
Member

commented Oct 12, 2017

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:

image

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.

@@ -11,6 +11,8 @@ import (
"net/http"
"reflect"
"testing"

"github.com/google/go-github/github"

This comment has been minimized.

Copy link
@dmitshur

dmitshur Oct 12, 2017

Member

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.

Jeremy Morris added some commits Oct 13, 2017

@googlebot

This comment has been minimized.

Copy link

commented Oct 13, 2017

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.

@googlebot googlebot added the cla: no label Oct 13, 2017

}

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

This comment has been minimized.

Copy link
@dmitshur

dmitshur Oct 13, 2017

Member

mediaTypeV3 is already the default set in NewRequest method:

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

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

// 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) {

This comment has been minimized.

Copy link
@dmitshur

dmitshur Oct 13, 2017

Member

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.:

// 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) {

This comment has been minimized.

Copy link
@dmitshur

dmitshur Oct 13, 2017

Member

Similarly, this would be RemoveRepository.

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

This comment has been minimized.

Copy link
@dmitshur

dmitshur Oct 13, 2017

Member

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

This comment has been minimized.

Copy link

commented Oct 13, 2017

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Oct 13, 2017

@dmitshur
Copy link
Member

left a comment

Really minor remaining things.


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

This comment has been minimized.

Copy link
@dmitshur

dmitshur Oct 13, 2017

Member

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


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)

This comment has been minimized.

Copy link
@dmitshur

dmitshur Oct 13, 2017

Member

And here.


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

This comment has been minimized.

Copy link
@dmitshur

dmitshur Oct 13, 2017

Member

As well as here.

Jeremy Morris added some commits Oct 13, 2017

Jeremy Morris
@dmitshur
Copy link
Member

left a comment

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!

//
// 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)

This comment has been minimized.

Copy link
@dmitshur

dmitshur Oct 13, 2017

Member

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)
//
// 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)

This comment has been minimized.

Copy link
@dmitshur

dmitshur Oct 13, 2017

Member

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

@dmitshur dmitshur requested review from gmlewis and elliott-beach Oct 13, 2017

Jeremy Morris
@MorrisLaw

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 13, 2017

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

@dmitshur
Copy link
Member

left a comment

LGTM. I'm not spotting any other issues.

Thank you @MorrisLaw!

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

@gmlewis
Copy link
Collaborator

left a comment

LGTM.
Thank you, @MorrisLaw and @shurcooL!

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

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.