From b24dbc902a0f6f2190bfcdcc3ab1ae85eb4a6289 Mon Sep 17 00:00:00 2001 From: Abhay Krishna Arunachalam Date: Mon, 6 Mar 2023 14:14:42 -0800 Subject: [PATCH] Ensure all GitHub releases are fetched when searching provider versions --- .../client/repository/repository_github.go | 26 +++++- .../repository/repository_github_test.go | 85 +++++++++++++++++-- 2 files changed, 100 insertions(+), 11 deletions(-) diff --git a/cmd/clusterctl/client/repository/repository_github.go b/cmd/clusterctl/client/repository/repository_github.go index 38f4b12cdcb8..04d7e99f6bbc 100644 --- a/cmd/clusterctl/client/repository/repository_github.go +++ b/cmd/clusterctl/client/repository/repository_github.go @@ -280,11 +280,12 @@ func (g *gitHubRepository) getVersions() ([]string, error) { // get all the releases // NB. currently Github API does not support result ordering, so it not possible to limit results - var releases []*github.RepositoryRelease + var allReleases []*github.RepositoryRelease var retryError error _ = wait.PollImmediate(retryableOperationInterval, retryableOperationTimeout, func() (bool, error) { var listReleasesErr error - releases, _, listReleasesErr = client.Repositories.ListReleases(context.TODO(), g.owner, g.repository, nil) + // Get the first page of GitHub releases + releases, response, listReleasesErr := client.Repositories.ListReleases(context.TODO(), g.owner, g.repository, nil) if listReleasesErr != nil { retryError = g.handleGithubErr(listReleasesErr, "failed to get the list of releases") // return immediately if we are rate limited @@ -293,6 +294,25 @@ func (g *gitHubRepository) getVersions() ([]string, error) { } return false, nil } + allReleases = append(allReleases, releases...) + + // https://github.com/google/go-github/blob/master/github/github.go#L536-L546 + if response.NextPage != 0 && response.LastPage != 0 { + nextPage := response.NextPage + lastPage := response.LastPage + for pageNum := nextPage; pageNum <= lastPage; pageNum++ { + releases, _, listReleasesErr = client.Repositories.ListReleases(context.TODO(), g.owner, g.repository, &github.ListOptions{Page: pageNum}) + if listReleasesErr != nil { + retryError = g.handleGithubErr(listReleasesErr, "failed to get the list of releases") + // return immediately if we are rate limited + if _, ok := listReleasesErr.(*github.RateLimitError); ok { + return false, retryError + } + return false, nil + } + allReleases = append(allReleases, releases...) + } + } retryError = nil return true, nil }) @@ -300,7 +320,7 @@ func (g *gitHubRepository) getVersions() ([]string, error) { return nil, retryError } versions := []string{} - for _, r := range releases { + for _, r := range allReleases { r := r // pin if r.TagName == nil { continue diff --git a/cmd/clusterctl/client/repository/repository_github_test.go b/cmd/clusterctl/client/repository/repository_github_test.go index a24631102704..067cd1018f45 100644 --- a/cmd/clusterctl/client/repository/repository_github_test.go +++ b/cmd/clusterctl/client/repository/repository_github_test.go @@ -21,6 +21,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "strings" "testing" "time" @@ -337,15 +338,46 @@ func Test_gitHubRepository_getVersions(t *testing.T) { client, mux, teardown := test.NewFakeGitHub() defer teardown() - // setup an handler for returning 5 fake releases + // Building a fake GitHubreleases array + releases := []string{} + for i := 0; i <= 10; i++ { + releases = append(releases, fmt.Sprintf(`{"id":%d, "tag_name": "v0.4.%d"}`, i+1, i)) + } + releases = append(releases, `{"id":4, "tag_name": "v0.4.3-alpha"}`) // prerelease + releases = append(releases, `{"id":5, "tag_name": "foo"}`) // no semantic version tag + + pageSize := 3 + validPageNum := 4 + invalidPageNum := 6 + + // setup a handler for returning paginated GitHub releases given a valid page number mux.HandleFunc("/repos/o/r1/releases", func(w http.ResponseWriter, r *http.Request) { testMethod(t, r, "GET") fmt.Fprint(w, `[`) - fmt.Fprint(w, `{"id":1, "tag_name": "v0.4.0"},`) - fmt.Fprint(w, `{"id":2, "tag_name": "v0.4.1"},`) - fmt.Fprint(w, `{"id":3, "tag_name": "v0.4.2"},`) - fmt.Fprint(w, `{"id":4, "tag_name": "v0.4.3-alpha"},`) // prerelease - fmt.Fprint(w, `{"id":5, "tag_name": "foo"}`) // no semantic version tag + fmt.Fprint(w, returnPageResults(releases, pageSize, validPageNum)) + fmt.Fprint(w, `]`) + }) + + // setup a handler for returning paginated GitHub releases given an invalid page number + mux.HandleFunc("/repos/o/r2/releases", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + fmt.Fprint(w, `[`) + fmt.Fprint(w, returnPageResults(releases, pageSize, invalidPageNum)) + fmt.Fprint(w, `]`) + }) + + // setup a handler for returning all pages of GitHub releases + mux.HandleFunc("/repos/o/r3/releases", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + fmt.Fprint(w, `[`) + numPages := len(releases) / pageSize + if len(releases)%pageSize > 0 { + numPages++ + } + for i := 1; i < numPages; i++ { + fmt.Fprintf(w, "%s,", returnPageResults(releases, pageSize, i)) + } + fmt.Fprint(w, returnPageResults(releases, pageSize, numPages)) fmt.Fprint(w, `]`) }) @@ -361,11 +393,27 @@ func Test_gitHubRepository_getVersions(t *testing.T) { wantErr bool }{ { - name: "Get versions", + name: "Get versions with valid page num", field: field{ providerConfig: config.NewProvider("test", "https://github.com/o/r1/releases/v0.4.1/path", clusterctlv1.CoreProviderType), }, - want: []string{"v0.4.0", "v0.4.1", "v0.4.2", "v0.4.3-alpha"}, + want: []string{"v0.4.9", "v0.4.10", "v0.4.3-alpha"}, + wantErr: false, + }, + { + name: "Get versions with invalid page num", + field: field{ + providerConfig: config.NewProvider("test", "https://github.com/o/r2/releases/v0.4.2/path", clusterctlv1.CoreProviderType), + }, + want: []string{}, + wantErr: false, + }, + { + name: "Get versions with all releases", + field: field{ + providerConfig: config.NewProvider("test", "https://github.com/o/r3/releases/v0.4.3/path", clusterctlv1.CoreProviderType), + }, + want: []string{"v0.4.0", "v0.4.1", "v0.4.2", "v0.4.3", "v0.4.4", "v0.4.5", "v0.4.6", "v0.4.7", "v0.4.8", "v0.4.9", "v0.4.10", "v0.4.3-alpha"}, wantErr: false, }, } @@ -876,3 +924,24 @@ func newFakeGoproxy() (client *goproxy.Client, mux *http.ServeMux, teardown func url, _ := url.Parse(server.URL + "/") return goproxy.NewClient(url.Scheme, url.Host), mux, server.Close } + +func min(a, b int) int { + if a < b { + return a + } + + return b +} + +func returnPageResults(releases []string, pageSize, pageNum int) string { + var sb strings.Builder + i := pageSize * (pageNum - 1) + if i < len(releases) { + for ; i < min(len(releases), pageSize*pageNum)-1; i++ { + sb.WriteString(fmt.Sprintf("%s,", releases[i])) + } + sb.WriteString(releases[i]) + } + + return sb.String() +}