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

fixes issues 26 & 46. Added support for pagination #83

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
85 changes: 64 additions & 21 deletions github.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,31 +106,41 @@ func FetchTags(githubRepoUrl string, githubToken string, instance GitHubInstance
return tagsString, wrapError(err)
}

url := createGitHubRepoUrlForPath(repo, "tags")
resp, err := callGitHubApi(repo, url, map[string]string{})
if err != nil {
return tagsString, err
}
//per_page is max to reduce network calls
for currPath := "tags?per_page=100"; currPath != ""; {
url := createGitHubRepoUrlForPath(repo, currPath)
resp, err := callGitHubApi(repo, url, map[string]string{})
if err != nil {
return tagsString, err
}

// Convert the response body to a byte array
buf := new(bytes.Buffer)
_, goErr := buf.ReadFrom(resp.Body)
if goErr != nil {
return tagsString, wrapError(goErr)
}
jsonResp := buf.Bytes()
// Convert the response body to a byte array
buf := new(bytes.Buffer)
_, goErr := buf.ReadFrom(resp.Body)
if goErr != nil {
return tagsString, wrapError(goErr)
}
jsonResp := buf.Bytes()

// Extract the JSON into our array of gitHubTagsCommitApiResponse's
var tags []GitHubTagsApiResponse
if err := json.Unmarshal(jsonResp, &tags); err != nil {
return tagsString, wrapError(err)
}
// Extract the JSON into our array of gitHubTagsCommitApiResponse's
var tags []GitHubTagsApiResponse
if err := json.Unmarshal(jsonResp, &tags); err != nil {
return tagsString, wrapError(err)
}

for _, tag := range tags {
// Skip tags that are not semantically versioned so that they don't cause errors. (issue #75)
if _, err := version.NewVersion(tag.Name); err == nil {
tagsString = append(tagsString, tag.Name)
for _, tag := range tags {
// Skip tags that are not semantically versioned so that they don't cause errors. (issue #75)
if _, err := version.NewVersion(tag.Name); err == nil {
tagsString = append(tagsString, tag.Name)
}
}

//Get paginated tags (issue #26 and #46)
currPath, err = getNextPath(resp.Header.Get("link"))
if err != nil {
return tagsString, err
}

}

return tagsString, nil
Expand Down Expand Up @@ -202,6 +212,39 @@ func createGitHubRepoUrlForPath(repo GitHubRepo, path string) string {
return fmt.Sprintf("repos/%s/%s/%s", repo.Owner, repo.Name, path)
}

// Get the Next paginated path from the link url api.github.com/repos/:owner/:repo/:path
// If there is no next page, return an empty string
// Links are formatted: "<url>; rel=next, <url>; rel=last"
func getNextPath(links string) (string, *FetchError) {
if len(links) == 0 {
return "", nil
}

nextLinkRegex, regexErr := regexp.Compile(`<(.+?)>;\s+rel="next"`)
Copy link
Member

Choose a reason for hiding this comment

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

In the regex here, it's rel="next" (in double quotes), but in the comment on line 217, it's rel=next (no double quotes). Which is correct?

Copy link
Member

Choose a reason for hiding this comment

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

NIT: Also, this regex can be defined outside of this function once (e.g., at the top of github.go), with MustCompile, rather than each time this function is called.

if regexErr != nil {
return "", wrapError(regexErr)
}
pathRegex, regexErr := regexp.Compile(`([^\/]+$)`)
Copy link
Member

Choose a reason for hiding this comment

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

NIT: same with compiling this one outside the function.

if regexErr != nil {
return "", wrapError(regexErr)
}

for _, link := range strings.Split(links, ",") {

urlMatches := nextLinkRegex.FindStringSubmatch(link)
if len(urlMatches) == 2 {
// Get only the next link path
pathMatches := pathRegex.FindStringSubmatch(urlMatches[1])
if len(pathMatches) != 2 {
return "", newError(githubRepoUrlMalformedOrNotParseable, fmt.Sprintf("Path parsed incorrectly for url: %s", urlMatches[1]))
}
return pathMatches[1], nil
}
}

return "", nil
}

// Call the GitHub API at the given path and return the HTTP response
func callGitHubApi(repo GitHubRepo, path string, customHeaders map[string]string) (*http.Response, *FetchError) {
httpClient := &http.Client{}
Expand Down
27 changes: 27 additions & 0 deletions github_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,33 @@ func TestGetListOfReleasesFromGitHubRepo(t *testing.T) {
}
}

func TestGetNextPath(t *testing.T) {
t.Parallel()

cases := []struct {
links string
expectedNextPath string
}{
{`<https://api.github.com/search/code?q=addClass+user%3Amozilla&page=15>; rel="next", <https://api.github.com/search/code?q=addClass+user%3Amozilla&page=34>; rel="last"`, "code?q=addClass+user%3Amozilla&page=15"},
{`<https://api.github.com/search/code?q=addClass+user%3Amozilla&page=15>; rel="first", <https://api.github.com/search/code?q=addClass+user%3Amozilla&page=34>; rel="last"`, ""},
{`<https://api.github.com/temp/proj/tags?page=15>; rel="next", <https://api.github.com/temp/proj/tags?page=15>; rel="last"`, "tags?page=15"},
{`<https://api.github.com/temp/proj/tags?per_page=100&page=15>; rel="next", <https://api.github.com/temp/proj/tags?per_page=100&page=15>; rel="last"`, "tags?per_page=100&page=15"},
}

for _, tc := range cases {
nextPath, err := getNextPath(tc.links)
Copy link
Member

Choose a reason for hiding this comment

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

NIT: wrap the body of the for-loop in:

t.Run("<UNIQUE TEST NAME>", func(t *testing.T) {
  // The body
})

That way, each test is run as a separate sub-test, and it's clearer what failed.


if err != nil {
t.Fatalf("error getting next path: %s", err)
}
Comment on lines +76 to +78
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if err != nil {
t.Fatalf("error getting next path: %s", err)
}
require.NoError(t, err)


if nextPath != tc.expectedNextPath {
t.Fatalf("Expected next path %s, but got %s", tc.expectedNextPath, nextPath)
}
Comment on lines +80 to +82
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if nextPath != tc.expectedNextPath {
t.Fatalf("Expected next path %s, but got %s", tc.expectedNextPath, nextPath)
}
require.Equal(t, tc.expectedNextPath, nextPath)

}

}

func TestParseUrlIntoGithubInstance(t *testing.T) {
t.Parallel()

Expand Down