Skip to content

Commit 79153f6

Browse files
abhay-krishnaabhay-krishna
committed
Ensure all GitHub releases are fetched when searching provider versions
1 parent 96e4fd6 commit 79153f6

File tree

2 files changed

+82
-43
lines changed

2 files changed

+82
-43
lines changed

‎cmd/clusterctl/client/repository/repository_github.go

Lines changed: 36 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -42,10 +42,11 @@ import (
4242
)
4343

4444
const (
45-
httpsScheme = "https"
46-
githubDomain = "github.com"
47-
githubReleaseRepository = "releases"
48-
githubLatestReleaseLabel = "latest"
45+
httpsScheme = "https"
46+
githubDomain = "github.com"
47+
githubReleaseRepository = "releases"
48+
githubLatestReleaseLabel = "latest"
49+
githubListReleasesPerPageLimit = 100
4950
)
5051

5152
var (
@@ -158,7 +159,7 @@ func (g *gitHubRepository) GetFile(version, path string) ([]byte, error) {
158159
return nil, errors.Wrapf(err, "failed to get GitHub release %s", version)
159160
}
160161

161-
// download files from the release
162+
// Download files from the release.
162163
files, err := g.downloadFilesFromRelease(release, path)
163164
if err != nil {
164165
return nil, errors.Wrapf(err, "failed to download files from GitHub release %s", version)
@@ -199,9 +200,9 @@ func NewGitHubRepository(providerConfig config.Provider, configVariablesClient c
199200
defaultVersion := urlSplit[3]
200201
path := strings.Join(urlSplit[4:], "/")
201202

202-
// use path's directory as a rootPath
203+
// Use path's directory as a rootPath.
203204
rootPath := filepath.Dir(path)
204-
// use the file name (if any) as componentsPath
205+
// Use the file name (if any) as componentsPath.
205206
componentsPath := getComponentsPath(path, rootPath)
206207

207208
repo := &gitHubRepository{
@@ -214,7 +215,7 @@ func NewGitHubRepository(providerConfig config.Provider, configVariablesClient c
214215
componentsPath: componentsPath,
215216
}
216217

217-
// process githubRepositoryOptions
218+
// Process githubRepositoryOptions.
218219
for _, o := range opts {
219220
o(repo)
220221
}
@@ -278,29 +279,47 @@ func (g *gitHubRepository) setClientToken(token string) {
278279
func (g *gitHubRepository) getVersions() ([]string, error) {
279280
client := g.getClient()
280281

281-
// get all the releases
282+
// Get all the releases.
282283
// NB. currently Github API does not support result ordering, so it not possible to limit results
283-
var releases []*github.RepositoryRelease
284+
var allReleases []*github.RepositoryRelease
284285
var retryError error
285286
_ = wait.PollImmediate(retryableOperationInterval, retryableOperationTimeout, func() (bool, error) {
286287
var listReleasesErr error
287-
releases, _, listReleasesErr = client.Repositories.ListReleases(context.TODO(), g.owner, g.repository, nil)
288+
// Get the first page of GitHub releases.
289+
releases, response, listReleasesErr := client.Repositories.ListReleases(context.TODO(), g.owner, g.repository, &github.ListOptions{PerPage: githubListReleasesPerPageLimit})
288290
if listReleasesErr != nil {
289291
retryError = g.handleGithubErr(listReleasesErr, "failed to get the list of releases")
290-
// return immediately if we are rate limited
292+
// Return immediately if we are rate limited.
291293
if _, ok := listReleasesErr.(*github.RateLimitError); ok {
292294
return false, retryError
293295
}
294296
return false, nil
295297
}
298+
allReleases = append(allReleases, releases...)
299+
300+
// Paginated GitHub APIs provide pointers to the first, next, previous and last
301+
// pages in the response, which can be used to iterate through the pages.
302+
// https://github.com/google/go-github/blob/14bb610698fc2f9013cad5db79b2d5fe4d53e13c/github/github.go#L541-L551
303+
for response.NextPage != 0 {
304+
releases, response, listReleasesErr = client.Repositories.ListReleases(context.TODO(), g.owner, g.repository, &github.ListOptions{Page: response.NextPage, PerPage: githubListReleasesPerPageLimit})
305+
if listReleasesErr != nil {
306+
retryError = g.handleGithubErr(listReleasesErr, "failed to get the list of releases")
307+
// Return immediately if we are rate limited.
308+
if _, ok := listReleasesErr.(*github.RateLimitError); ok {
309+
return false, retryError
310+
}
311+
return false, nil
312+
}
313+
allReleases = append(allReleases, releases...)
314+
}
296315
retryError = nil
297316
return true, nil
298317
})
299318
if retryError != nil {
300319
return nil, retryError
301320
}
302321
versions := []string{}
303-
for _, r := range releases {
322+
for _, r := range allReleases {
304323
r := r // pin
305324
if r.TagName == nil {
306325
continue
@@ -332,7 +351,7 @@ func (g *gitHubRepository) getReleaseByTag(tag string) (*github.RepositoryReleas
332351
release, _, getReleasesErr = client.Repositories.GetReleaseByTag(context.TODO(), g.owner, g.repository, tag)
333352
if getReleasesErr != nil {
334353
retryError = g.handleGithubErr(getReleasesErr, "failed to read release %q", tag)
335-
// return immediately if we are rate limited
354+
// Return immediately if we are rate limited.
336355
if _, ok := getReleasesErr.(*github.RateLimitError); ok {
337356
return false, retryError
338357
}
@@ -361,7 +380,7 @@ func (g *gitHubRepository) downloadFilesFromRelease(release *github.RepositoryRe
361380
client := g.getClient()
362381
absoluteFileName := filepath.Join(g.rootPath, fileName)
363382

364-
// search for the file into the release assets, retrieving the asset id
383+
// Search for the file into the release assets, retrieving the asset id.
365384
var assetID *int64
366385
for _, a := range release.Assets {
367386
if a.Name != nil && *a.Name == absoluteFileName {
@@ -381,14 +400,14 @@ func (g *gitHubRepository) downloadFilesFromRelease(release *github.RepositoryRe
381400
reader, redirect, downloadReleaseError = client.Repositories.DownloadReleaseAsset(ctx, g.owner, g.repository, *assetID, http.DefaultClient)
382401
if downloadReleaseError != nil {
383402
retryError = g.handleGithubErr(downloadReleaseError, "failed to download file %q from %q release", *release.TagName, fileName)
384-
// return immediately if we are rate limited
403+
// Return immediately if we are rate limited.
385404
if _, ok := downloadReleaseError.(*github.RateLimitError); ok {
386405
return false, retryError
387406
}
388407
return false, nil
389408
}
390409
if redirect != "" {
391-
// NOTE: DownloadReleaseAsset should not return a redirect address when used with the DefaultClient
410+
// NOTE: DownloadReleaseAsset should not return a redirect address when used with the DefaultClient.
392411
retryError = errors.New("unexpected redirect while downloading the release asset")
393412
return true, retryError
394413
}

‎cmd/clusterctl/client/repository/repository_github_test.go

Lines changed: 46 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -41,21 +41,21 @@ func Test_gitHubRepository_GetVersions(t *testing.T) {
4141
client, mux, teardown := test.NewFakeGitHub()
4242
defer teardown()
4343

44-
// setup an handler for returning 5 fake releases
44+
// Setup an handler for returning 5 fake releases.
4545
mux.HandleFunc("/repos/o/r1/releases", func(w http.ResponseWriter, r *http.Request) {
4646
testMethod(t, r, "GET")
4747
fmt.Fprint(w, `[`)
4848
fmt.Fprint(w, `{"id":1, "tag_name": "v0.4.0"},`)
4949
fmt.Fprint(w, `{"id":2, "tag_name": "v0.4.1"},`)
5050
fmt.Fprint(w, `{"id":3, "tag_name": "v0.4.2"},`)
51-
fmt.Fprint(w, `{"id":4, "tag_name": "v0.4.3-alpha"}`) // prerelease
51+
fmt.Fprint(w, `{"id":4, "tag_name": "v0.4.3-alpha"}`) // Pre-release
5252
fmt.Fprint(w, `]`)
5353
})
5454

5555
clientGoproxy, muxGoproxy, teardownGoproxy := newFakeGoproxy()
5656
defer teardownGoproxy()
5757

58-
// setup an handler for returning 4 fake releases
58+
// Setup a handler for returning 4 fake releases.
5959
muxGoproxy.HandleFunc("/github.com/o/r2/@v/list", func(w http.ResponseWriter, r *http.Request) {
6060
testMethod(t, r, "GET")
6161
fmt.Fprint(w, "v0.5.0\n")
@@ -64,7 +64,7 @@ func Test_gitHubRepository_GetVersions(t *testing.T) {
6464
fmt.Fprint(w, "v0.3.1\n")
6565
})
6666

67-
// setup an handler for returning 3 different major fake releases
67+
// Setup a handler for returning 3 different major fake releases.
6868
muxGoproxy.HandleFunc("/github.com/o/r3/@v/list", func(w http.ResponseWriter, r *http.Request) {
6969
testMethod(t, r, "GET")
7070
fmt.Fprint(w, "v1.0.0\n")
@@ -265,13 +265,13 @@ func Test_githubRepository_getFile(t *testing.T) {
265265

266266
providerConfig := config.NewProvider("test", "https://github.com/o/r/releases/v0.4.1/file.yaml", clusterctlv1.CoreProviderType)
267267

268-
// test.NewFakeGitHub and handler for returning a fake release
268+
// Setup a handler for returning a fake release.
269269
mux.HandleFunc("/repos/o/r/releases/tags/v0.4.1", func(w http.ResponseWriter, r *http.Request) {
270270
testMethod(t, r, "GET")
271271
fmt.Fprint(w, `{"id":13, "tag_name": "v0.4.1", "assets": [{"id": 1, "name": "file.yaml"}] }`)
272272
})
273273

274-
// test.NewFakeGitHub an handler for returning a fake release asset
274+
// Setup a handler for returning a fake release asset.
275275
mux.HandleFunc("/repos/o/r/releases/assets/1", func(w http.ResponseWriter, r *http.Request) {
276276
testMethod(t, r, "GET")
277277
w.Header().Set("Content-Type", "application/octet-stream")
@@ -337,16 +337,36 @@ func Test_gitHubRepository_getVersions(t *testing.T) {
337337
client, mux, teardown := test.NewFakeGitHub()
338338
defer teardown()
339339

340-
// setup an handler for returning 5 fake releases
340+
// Setup a handler for returning fake releases in a paginated manner
341+
// Each response contains a link to the next page (if available) which
342+
// is parsed by the handler to navigate through all pages
341343
mux.HandleFunc("/repos/o/r1/releases", func(w http.ResponseWriter, r *http.Request) {
342344
testMethod(t, r, "GET")
343-
fmt.Fprint(w, `[`)
344-
fmt.Fprint(w, `{"id":1, "tag_name": "v0.4.0"},`)
345-
fmt.Fprint(w, `{"id":2, "tag_name": "v0.4.1"},`)
346-
fmt.Fprint(w, `{"id":3, "tag_name": "v0.4.2"},`)
347-
fmt.Fprint(w, `{"id":4, "tag_name": "v0.4.3-alpha"},`) // prerelease
348-
fmt.Fprint(w, `{"id":5, "tag_name": "foo"}`) // no semantic version tag
349-
fmt.Fprint(w, `]`)
345+
page := r.URL.Query().Get("page")
346+
switch page {
347+
case "", "1":
348+
// Page 1
349+
w.Header().Set("Link", `<https://api.github.com/repositories/12345/releases?page=2>; rel="next"`) // Link to page 2
350+
fmt.Fprint(w, `[`)
351+
fmt.Fprint(w, `{"id":1, "tag_name": "v0.4.0"},`)
352+
fmt.Fprint(w, `{"id":2, "tag_name": "v0.4.1"}`)
353+
fmt.Fprint(w, `]`)
354+
case "2":
355+
// Page 2
356+
w.Header().Set("Link", `<https://api.github.com/repositories/12345/releases?page=3>; rel="next"`) // Link to page 3
357+
fmt.Fprint(w, `[`)
358+
fmt.Fprint(w, `{"id":3, "tag_name": "v0.4.2"},`)
359+
fmt.Fprint(w, `{"id":4, "tag_name": "v0.4.3-alpha"}`) // Pre-release
360+
fmt.Fprint(w, `]`)
361+
case "3":
362+
// Page 3 (last page)
363+
fmt.Fprint(w, `[`)
364+
fmt.Fprint(w, `{"id":4, "tag_name": "v0.4.4-beta"},`) // Pre-release
365+
fmt.Fprint(w, `{"id":5, "tag_name": "foo"}`) // No semantic version tag
366+
fmt.Fprint(w, `]`)
367+
default:
368+
t.Fatalf("unexpected page requested")
369+
}
350370
})
351371

352372
configVariablesClient := test.NewFakeVariableClient()
@@ -361,11 +381,11 @@ func Test_gitHubRepository_getVersions(t *testing.T) {
361381
wantErr bool
362382
}{
363383
{
364-
name: "Get versions",
384+
name: "Get versions with all releases",
365385
field: field{
366386
providerConfig: config.NewProvider("test", "https://github.com/o/r1/releases/v0.4.1/path", clusterctlv1.CoreProviderType),
367387
},
368-
want: []string{"v0.4.0", "v0.4.1", "v0.4.2", "v0.4.3-alpha"},
388+
want: []string{"v0.4.0", "v0.4.1", "v0.4.2", "v0.4.3-alpha", "v0.4.4-beta"},
369389
wantErr: false,
370390
},
371391
}
@@ -395,13 +415,13 @@ func Test_gitHubRepository_getLatestContractRelease(t *testing.T) {
395415
client, mux, teardown := test.NewFakeGitHub()
396416
defer teardown()
397417

398-
// test.NewFakeGitHub and handler for returning a fake release
418+
// Setup a handler for returning a fake release.
399419
mux.HandleFunc("/repos/o/r1/releases/tags/v0.5.0", func(w http.ResponseWriter, r *http.Request) {
400420
testMethod(t, r, "GET")
401421
fmt.Fprint(w, `{"id":13, "tag_name": "v0.5.0", "assets": [{"id": 1, "name": "metadata.yaml"}] }`)
402422
})
403423

404-
// test.NewFakeGitHub an handler for returning a fake release metadata file
424+
// Setup a handler for returning a fake release metadata file.
405425
mux.HandleFunc("/repos/o/r1/releases/assets/1", func(w http.ResponseWriter, r *http.Request) {
406426
testMethod(t, r, "GET")
407427
w.Header().Set("Content-Type", "application/octet-stream")
@@ -412,7 +432,7 @@ func Test_gitHubRepository_getLatestContractRelease(t *testing.T) {
412432
clientGoproxy, muxGoproxy, teardownGoproxy := newFakeGoproxy()
413433
defer teardownGoproxy()
414434

415-
// setup an handler for returning 4 fake releases
435+
// Setup a handler for returning 4 fake releases.
416436
muxGoproxy.HandleFunc("/github.com/o/r1/@v/list", func(w http.ResponseWriter, r *http.Request) {
417437
testMethod(t, r, "GET")
418438
fmt.Fprint(w, "v0.5.0\n")
@@ -486,7 +506,7 @@ func Test_gitHubRepository_getLatestRelease(t *testing.T) {
486506
clientGoproxy, muxGoproxy, teardownGoproxy := newFakeGoproxy()
487507
defer teardownGoproxy()
488508

489-
// setup an handler for returning 4 fake releases
509+
// Setup a handler for returning 4 fake releases.
490510
muxGoproxy.HandleFunc("/github.com/o/r1/@v/list", func(w http.ResponseWriter, r *http.Request) {
491511
testMethod(t, r, "GET")
492512
fmt.Fprint(w, "v0.4.1\n")
@@ -495,13 +515,13 @@ func Test_gitHubRepository_getLatestRelease(t *testing.T) {
495515
fmt.Fprint(w, "foo\n") // no semantic version tag
496516
})
497517

498-
// setup an handler for returning no releases
518+
// Setup a handler for returning no releases.
499519
muxGoproxy.HandleFunc("/github.com/o/r2/@v/list", func(w http.ResponseWriter, r *http.Request) {
500520
testMethod(t, r, "GET")
501521
// no releases
502522
})
503523

504-
// setup an handler for returning fake prereleases only
524+
// Setup a handler for returning fake prereleases only.
505525
muxGoproxy.HandleFunc("/github.com/o/r3/@v/list", func(w http.ResponseWriter, r *http.Request) {
506526
testMethod(t, r, "GET")
507527
fmt.Fprint(w, "v0.1.0-alpha.0\n")
@@ -571,7 +591,7 @@ func Test_gitHubRepository_getLatestPatchRelease(t *testing.T) {
571591
clientGoproxy, muxGoproxy, teardownGoproxy := newFakeGoproxy()
572592
defer teardownGoproxy()
573593

574-
// setup an handler for returning 4 fake releases
594+
// Setup a handler for returning 4 fake releases.
575595
muxGoproxy.HandleFunc("/github.com/o/r1/@v/list", func(w http.ResponseWriter, r *http.Request) {
576596
testMethod(t, r, "GET")
577597
fmt.Fprint(w, "v0.4.0\n")
@@ -654,7 +674,7 @@ func Test_gitHubRepository_getReleaseByTag(t *testing.T) {
654674

655675
providerConfig := config.NewProvider("test", "https://github.com/o/r/releases/v0.4.1/path", clusterctlv1.CoreProviderType)
656676

657-
// setup and handler for returning a fake release
677+
// Setup a handler for returning a fake release.
658678
mux.HandleFunc("/repos/o/r/releases/tags/foo", func(w http.ResponseWriter, r *http.Request) {
659679
testMethod(t, r, "GET")
660680
fmt.Fprint(w, `{"id":13, "tag_name": "v0.4.1"}`)
@@ -722,14 +742,14 @@ func Test_gitHubRepository_downloadFilesFromRelease(t *testing.T) {
722742
providerConfig := config.NewProvider("test", "https://github.com/o/r/releases/v0.4.1/file.yaml", clusterctlv1.CoreProviderType) // tree/main/path not relevant for the test
723743
providerConfigWithRedirect := config.NewProvider("test", "https://github.com/o/r-with-redirect/releases/v0.4.1/file.yaml", clusterctlv1.CoreProviderType) // tree/main/path not relevant for the test
724744

725-
// test.NewFakeGitHub an handler for returning a fake release asset
745+
// Setup a handler for returning a fake release asset.
726746
mux.HandleFunc("/repos/o/r/releases/assets/1", func(w http.ResponseWriter, r *http.Request) {
727747
testMethod(t, r, "GET")
728748
w.Header().Set("Content-Type", "application/octet-stream")
729749
w.Header().Set("Content-Disposition", "attachment; filename=file.yaml")
730750
fmt.Fprint(w, "content")
731751
})
732-
// handler which redirects to a different location
752+
// Setup a handler which redirects to a different location.
733753
mux.HandleFunc("/repos/o/r-with-redirect/releases/assets/1", func(w http.ResponseWriter, r *http.Request) {
734754
testMethod(t, r, "GET")
735755
http.Redirect(w, r, "/api-v3/repos/o/r/releases/assets/1", http.StatusFound)

0 commit comments

Comments
 (0)