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
Merged
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
aa10fb4
Added my name to AUTHORS list.
MorrisLaw Aug 22, 2017
7772ba0
Added API route for adding a repo to an installation.
MorrisLaw Aug 22, 2017
5d99cdf
Added api route for removing repo from installation.
MorrisLaw Aug 22, 2017
2eab2b3
Moved Add/Remove Repository functions to app_installation.
MorrisLaw Aug 22, 2017
a35a566
Simplified logic in remove repository.
MorrisLaw Aug 22, 2017
3e14143
Style fix.
MorrisLaw Aug 22, 2017
44e0f51
Refactored function names.
MorrisLaw Aug 22, 2017
defe8b8
Renamed functions back to having Repository suffix.
MorrisLaw Aug 22, 2017
f31595a
Change argument from Installation to Repository.
MorrisLaw Aug 22, 2017
5934515
Changed variable name for Repository.
MorrisLaw Aug 22, 2017
e20f019
Shortened function name.
MorrisLaw Aug 22, 2017
16bcc33
Added tests for Add and Remove repo.
MorrisLaw Aug 22, 2017
f90fa0d
Fixed golint issues.
MorrisLaw Aug 22, 2017
71f1694
Merge remote-tracking branch 'origin/master' into enhancement/add-rem…
MorrisLaw Aug 23, 2017
585efab
Removed merge conflict from adding my name to list.
MorrisLaw Aug 23, 2017
030ae55
Ran `go fmt` on file.
MorrisLaw Aug 23, 2017
11aefc3
Ran `go fmt` on file.
MorrisLaw Aug 23, 2017
173da1c
Changed s *AppService to s *AppsService .
MorrisLaw Aug 23, 2017
2c3db48
Added import and removed extra value from return in removeRepo.
MorrisLaw Aug 23, 2017
158fea8
Fixed some issues discovered by `go test` .
MorrisLaw Aug 23, 2017
0c7acc2
Merge branch 'master' into enhancement/add-remove-repository
MorrisLaw Aug 29, 2017
9b8d40c
Merge remote-tracking branch 'upstream/master' into enhancement/add-r…
Oct 4, 2017
b9c5873
Added colons to url path for add and remove repo.
Oct 4, 2017
e132615
Removed colons from http put/delete paths.
Oct 4, 2017
926a90c
Debugging broken test and using example from repos_test as building b…
Oct 4, 2017
f1bf61b
Merge branch 'master' into enhancement/add-remove-repository
MorrisLaw Oct 10, 2017
5e8d98c
Merge branch 'enhancement/add-remove-repository' of https://github.co…
MorrisLaw Oct 10, 2017
a612b5e
Refactored url string.
MorrisLaw Oct 10, 2017
f35c3c9
Add repo test passes.
Oct 11, 2017
d6c991e
Uncommented custom accept header.
Oct 11, 2017
4d5820a
Refactored method signatures add and remove repo. Added response chec…
Oct 11, 2017
b2f7773
Merge branch 'enhancement/add-remove-repository' of https://github.co…
Oct 11, 2017
60af6e6
Ran go fmt on codebase.
Oct 11, 2017
8e876e4
Removed unnecessary import and changed *github.ErrorResponse to *Erro…
Oct 13, 2017
42faa4d
Fixed url path for RemoveRepo and removed extra conditional in Remove…
Oct 13, 2017
326457b
Added newline at eof, to restart build after CLA email fix.
MorrisLaw Oct 13, 2017
b4b56c3
Removed newline from test file.
MorrisLaw Oct 13, 2017
432dab7
Fixed suggestions for naming functions, and fixing error check.
Oct 13, 2017
c01f313
Added full function names to Errorf strings, to match referenced func…
Oct 13, 2017
41d02d7
Removed mediatypev3.
Oct 13, 2017
b5f3a3b
Removed starting slash in URLs.
Oct 13, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
Expand Up @@ -77,6 +77,7 @@ i2bskn <i2bskn@gmail.com>
Isao Jonas <isao.jonas@gmail.com>
isqua <isqua@isqua.ru>
Jameel Haffejee <RC1140@republiccommandos.co.za>
Jeremy Morris <jeremylevanmorris@gmail.com>
Jan Kosecki <jan.kosecki91@gmail.com>
Jihoon Chung <j.c@navercorp.com>
Jimmi Dyson <jimmidyson@gmail.com>
Expand Down
40 changes: 39 additions & 1 deletion github/apps_installation.go
Expand Up @@ -5,7 +5,10 @@

package github

import "context"
import (
"context"
"fmt"
)

// Installation represents a GitHub Apps installation.
type Installation struct {
Expand Down Expand Up @@ -47,3 +50,38 @@ func (s *AppsService) ListRepos(ctx context.Context, opt *ListOptions) ([]*Repos

return r.Repositories, resp, nil
}

// 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

req, err := s.client.NewRequest("PUT", u, nil)
if err != nil {
return nil, nil, err
}

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

@dmitshur dmitshur Oct 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

@dmitshur dmitshur Oct 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

return r, resp, nil
}

// 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, this would be RemoveRepository.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

req, err := s.client.NewRequest("DELETE", u, nil)
if err != nil {
return nil, err
}

return s.client.Do(ctx, req, nil)
}
36 changes: 36 additions & 0 deletions github/apps_installation_test.go
Expand Up @@ -38,3 +38,39 @@ func TestAppsService_ListRepos(t *testing.T) {
t.Errorf("Apps.ListRepos returned %+v, want %+v", repositories, want)
}
}

func TestAppsService_AddRepo(t *testing.T) {
setup()
defer teardown()

mux.HandleFunc("/apps/installations/1/repositories/1", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "PUT")
testHeader(t, r, "Accept", mediaTypeV3)
fmt.Fprint(w, `{"id":1,"name":"n","description":"d","owner":{"login":"l"},"license":{"key":"mit"}}`)
})

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here.

}
}

func TestAppsService_RemoveRepo(t *testing.T) {
setup()
defer teardown()

mux.HandleFunc("/apps/installations/1/repositories/1", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "DELETE")
w.WriteHeader(http.StatusNoContent)
})

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As well as here.

}
}