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.

Sign up for GitHub

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

[PL-26239]: fix for list response #218

Merged
merged 3 commits into from
Aug 26, 2022
Merged

[PL-26239]: fix for list response #218

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e6ca341
[PL-26239]: fix for list response
bhavya181 Aug 23, 2022
40c58c1
[PL-26239]: fix UT
bhavya181 Aug 23, 2022
bae5ca9
[PL-26239]: changes done
bhavya181 Aug 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 14 additions & 9 deletions scm/driver/github/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (
"github.com/drone/go-scm/scm"
)

type repository struct {
type Repository struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I understand, it makes the struct public. Why do we need to make it public if all references are from same package/file?
Sorry, I may be naive here, just trying to understand its impact

Copy link
Contributor Author

Choose a reason for hiding this comment

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

json unmarshal and marshal only works when name of struct start with capital letter

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh interesting. Thanks for the info.

Copy link
Contributor

Choose a reason for hiding this comment

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

image

https://github.com//pull/219 repositories should be lower case as @mohitg0795 said

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but then, RepositoryListResponse will not be able to map to Repository struct as it does not unmarshal that struct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you suggest any other method, so that it will map correctly

Copy link
Contributor

Choose a reason for hiding this comment

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

can you show me where it doesnt work, the unit tests passed and it unmarshalled successfully

if you look at the do method for all drivers, it marshalls / unmarshalls without all of the structs. The other structs all start with a lowercase letter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya, I have changed them now

ID int `json:"id"`
Owner struct {
ID int `json:"id"`
Expand Down Expand Up @@ -53,6 +53,11 @@ type hook struct {
} `json:"config"`
}

type RepositoryListResponse struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

should start with a lowercase letter

Copy link
Member

@bradrydzewski bradrydzewski Aug 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

agree this should be lowercase. Also we do not suffix with Response so it should be repositoryList instead. If this package already has a repositoryList struct, you could alternative use the name repositoryListByInstallation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

TotalCount int `json:"total_count"`
Repositories []*Repository `json:"repositories"`
}

// RepositoryService implements the repository service for
// the GitHub driver.
type RepositoryService struct {
Expand All @@ -62,7 +67,7 @@ type RepositoryService struct {
// Find returns the repository by name.
func (s *RepositoryService) Find(ctx context.Context, repo string) (*scm.Repository, *scm.Response, error) {
path := fmt.Sprintf("repos/%s", repo)
out := new(repository)
out := new(Repository)
res, err := s.client.do(ctx, "GET", path, nil, out)
if err != nil {
return nil, res, err
Expand All @@ -85,7 +90,7 @@ func (s *RepositoryService) FindHook(ctx context.Context, repo string, id string
// FindPerms returns the repository permissions.
func (s *RepositoryService) FindPerms(ctx context.Context, repo string) (*scm.Perm, *scm.Response, error) {
path := fmt.Sprintf("repos/%s", repo)
out := new(repository)
out := new(Repository)
res, err := s.client.do(ctx, "GET", path, nil, out)
if err != nil {
return nil, res, err
Expand All @@ -100,17 +105,17 @@ func (s *RepositoryService) FindPerms(ctx context.Context, repo string) (*scm.Pe
// List returns the user repository list.
func (s *RepositoryService) List(ctx context.Context, opts scm.ListOptions) ([]*scm.Repository, *scm.Response, error) {
path := fmt.Sprintf("user/repos?%s", encodeListOptions(opts))
out := []*repository{}
out := []*Repository{}
res, err := s.client.do(ctx, "GET", path, nil, &out)
return convertRepositoryList(out), res, err
}

// List returns the github app installation repository list.
func (s *RepositoryService) ListByInstallation(ctx context.Context, opts scm.ListOptions) ([]*scm.Repository, *scm.Response, error) {
path := fmt.Sprintf("installation/repositories?%s", encodeListOptions(opts))
out := []*repository{}
res, err := s.client.do(ctx, "GET", path, nil, &out)
return convertRepositoryList(out), res, err
out := new(RepositoryListResponse)
res, err := s.client.do(ctx, "GET", path, nil, out)
return convertRepositoryList(out.Repositories), res, err
}

// ListHooks returns a list or repository hooks.
Expand Down Expand Up @@ -207,7 +212,7 @@ func (s *RepositoryService) DeleteHook(ctx context.Context, repo, id string) (*s

// helper function to convert from the gogs repository list to
// the common repository structure.
func convertRepositoryList(from []*repository) []*scm.Repository {
func convertRepositoryList(from []*Repository) []*scm.Repository {
to := []*scm.Repository{}
for _, v := range from {
if repo := convertRepository(v); repo != nil {
Expand All @@ -219,7 +224,7 @@ func convertRepositoryList(from []*repository) []*scm.Repository {

// helper function to convert from the gogs repository structure
// to the common repository structure.
func convertRepository(from *repository) *scm.Repository {
func convertRepository(from *Repository) *scm.Repository {
if from == nil {
return nil
}
Expand Down
8 changes: 4 additions & 4 deletions scm/driver/github/repo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ func TestGithubAppInstallationList(t *testing.T) {
Type("application/json").
SetHeaders(mockHeaders).
SetHeaders(mockPageHeaders).
File("testdata/repos.json")
File("testdata/github_app_repos.json")

client := NewDefault()
got, res, err := client.Repositories.(*RepositoryService).ListByInstallation(context.Background(), scm.ListOptions{Page: 1, Size: 30})
Expand Down Expand Up @@ -565,18 +565,18 @@ func TestHookEvents(t *testing.T) {
}

func Test_convertRepository(t *testing.T) {
permissionsStruct := &repository{
permissionsStruct := &Repository{
Name: "bla",
}
permissionsStruct.Permissions.Admin = true
tests := []struct {
name string
from *repository
from *Repository
want *scm.Repository
}{
{
name: "Simple",
from: &repository{
from: &Repository{
Name: "hello-world",
ID: 1,
Permissions: permissionsStruct.Permissions,
Expand Down
115 changes: 115 additions & 0 deletions scm/driver/github/testdata/github_app_repos.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
{
"total_count": 1,
"repositories": [
{
"id": 1296269,
"owner": {
"login": "octocat",
"id": 1,
"avatar_url": "https://github.com/images/error/octocat_happy.gif",
"gravatar_id": "",
"url": "https://api.github.com/users/octocat",
"html_url": "https://github.com/octocat",
"followers_url": "https://api.github.com/users/octocat/followers",
"following_url": "https://api.github.com/users/octocat/following{/other_user}",
"gists_url": "https://api.github.com/users/octocat/gists{/gist_id}",
"starred_url": "https://api.github.com/users/octocat/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/octocat/subscriptions",
"organizations_url": "https://api.github.com/users/octocat/orgs",
"repos_url": "https://api.github.com/users/octocat/repos",
"events_url": "https://api.github.com/users/octocat/events{/privacy}",
"received_events_url": "https://api.github.com/users/octocat/received_events",
"type": "User",
"site_admin": false
},
"name": "Hello-World",
"full_name": "octocat/Hello-World",
"description": "This your first repo!",
"private": true,
"fork": true,
"visibility": "public",
"url": "https://api.github.com/repos/octocat/Hello-World",
"html_url": "https://github.com/octocat/Hello-World",
"archive_url": "http://api.github.com/repos/octocat/Hello-World/{archive_format}{/ref}",
"assignees_url": "http://api.github.com/repos/octocat/Hello-World/assignees{/user}",
"blobs_url": "http://api.github.com/repos/octocat/Hello-World/git/blobs{/sha}",
"branches_url": "http://api.github.com/repos/octocat/Hello-World/branches{/branch}",
"clone_url": "https://github.com/octocat/Hello-World.git",
"collaborators_url": "http://api.github.com/repos/octocat/Hello-World/collaborators{/collaborator}",
"comments_url": "http://api.github.com/repos/octocat/Hello-World/comments{/number}",
"commits_url": "http://api.github.com/repos/octocat/Hello-World/commits{/sha}",
"compare_url": "http://api.github.com/repos/octocat/Hello-World/compare/{base}...{head}",
"contents_url": "http://api.github.com/repos/octocat/Hello-World/contents/{+path}",
"contributors_url": "http://api.github.com/repos/octocat/Hello-World/contributors",
"deployments_url": "http://api.github.com/repos/octocat/Hello-World/deployments",
"downloads_url": "http://api.github.com/repos/octocat/Hello-World/downloads",
"events_url": "http://api.github.com/repos/octocat/Hello-World/events",
"forks_url": "http://api.github.com/repos/octocat/Hello-World/forks",
"git_commits_url": "http://api.github.com/repos/octocat/Hello-World/git/commits{/sha}",
"git_refs_url": "http://api.github.com/repos/octocat/Hello-World/git/refs{/sha}",
"git_tags_url": "http://api.github.com/repos/octocat/Hello-World/git/tags{/sha}",
"git_url": "git:github.com/octocat/Hello-World.git",
"hooks_url": "http://api.github.com/repos/octocat/Hello-World/hooks",
"issue_comment_url": "http://api.github.com/repos/octocat/Hello-World/issues/comments{/number}",
"issue_events_url": "http://api.github.com/repos/octocat/Hello-World/issues/events{/number}",
"issues_url": "http://api.github.com/repos/octocat/Hello-World/issues{/number}",
"keys_url": "http://api.github.com/repos/octocat/Hello-World/keys{/key_id}",
"labels_url": "http://api.github.com/repos/octocat/Hello-World/labels{/name}",
"languages_url": "http://api.github.com/repos/octocat/Hello-World/languages",
"merges_url": "http://api.github.com/repos/octocat/Hello-World/merges",
"milestones_url": "http://api.github.com/repos/octocat/Hello-World/milestones{/number}",
"mirror_url": "git:git.example.com/octocat/Hello-World",
"notifications_url": "http://api.github.com/repos/octocat/Hello-World/notifications{?since, all, participating}",
"pulls_url": "http://api.github.com/repos/octocat/Hello-World/pulls{/number}",
"releases_url": "http://api.github.com/repos/octocat/Hello-World/releases{/id}",
"ssh_url": "git@github.com:octocat/Hello-World.git",
"stargazers_url": "http://api.github.com/repos/octocat/Hello-World/stargazers",
"statuses_url": "http://api.github.com/repos/octocat/Hello-World/statuses/{sha}",
"subscribers_url": "http://api.github.com/repos/octocat/Hello-World/subscribers",
"subscription_url": "http://api.github.com/repos/octocat/Hello-World/subscription",
"svn_url": "https://svn.github.com/octocat/Hello-World",
"tags_url": "http://api.github.com/repos/octocat/Hello-World/tags",
"teams_url": "http://api.github.com/repos/octocat/Hello-World/teams",
"trees_url": "http://api.github.com/repos/octocat/Hello-World/git/trees{/sha}",
"homepage": "https://github.com",
"language": null,
"forks_count": 9,
"stargazers_count": 80,
"watchers_count": 80,
"size": 108,
"default_branch": "master",
"open_issues_count": 0,
"topics": [
"octocat",
"atom",
"electron",
"API"
],
"has_issues": true,
"has_wiki": true,
"has_pages": false,
"has_downloads": true,
"archived": false,
"pushed_at": "2011-01-26T19:06:43Z",
"created_at": "2011-01-26T19:01:12Z",
"updated_at": "2011-01-26T19:14:43Z",
"permissions": {
"admin": true,
"push": true,
"pull": true
},
"allow_rebase_merge": true,
"allow_squash_merge": true,
"allow_merge_commit": true,
"subscribers_count": 42,
"network_count": 0,
"license": {
"key": "mit",
"name": "MIT License",
"spdx_id": "MIT",
"url": "https://api.github.com/licenses/mit",
"html_url": "http://choosealicense.com/licenses/mit/"
}
}
]
}
8 changes: 4 additions & 4 deletions scm/driver/github/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ type (
createDeleteHook struct {
Ref string `json:"ref"`
RefType string `json:"ref_type"`
Repository repository `json:"repository"`
Repository Repository `json:"repository"`
Sender user `json:"sender"`
}

Expand Down Expand Up @@ -264,7 +264,7 @@ type (
Action string `json:"action"`
Number int `json:"number"`
PullRequest pr `json:"pull_request"`
Repository repository `json:"repository"`
Repository Repository `json:"repository"`
Sender user `json:"sender"`
}

Expand All @@ -282,15 +282,15 @@ type (
Task null.String `json:"task"`
Payload interface{} `json:"payload"`
} `json:"deployment"`
Repository repository `json:"repository"`
Repository Repository `json:"repository"`
Sender user `json:"sender"`
}

// github issue_comment webhook payload
issueCommentHook struct {
Action string `json:"action"`
Issue issue `json:"issue"`
Repository repository `json:"repository"`
Repository Repository `json:"repository"`
Sender user `json:"sender"`
Organization user `json:"organization"`
Comment struct {
Expand Down