From 38d06c8f08b7c970e1135145547d5d7a4f2fe997 Mon Sep 17 00:00:00 2001 From: Sascha Grunert Date: Mon, 2 Mar 2020 14:54:16 +0100 Subject: [PATCH] Refactor kubepkg.fetchReleases into github.Releases() We move the `fetchReleases` function into the `github` package to achieve testablity and encapsulation of GitHub related functionality. Signed-off-by: Sascha Grunert --- pkg/github/github.go | 37 ++++++++++- pkg/github/github_test.go | 83 +++++++++++++++++++++++++ pkg/github/githubfakes/fake_client.go | 89 +++++++++++++++++++++++++++ pkg/kubepkg/BUILD.bazel | 2 +- pkg/kubepkg/kubepkg.go | 27 +------- 5 files changed, 211 insertions(+), 27 deletions(-) diff --git a/pkg/github/github.go b/pkg/github/github.go index cf902e279b0..bf26505aa0b 100644 --- a/pkg/github/github.go +++ b/pkg/github/github.go @@ -43,6 +43,10 @@ type Client interface { ListTags( context.Context, string, string, *github.ListOptions, ) ([]*github.RepositoryTag, *github.Response, error) + + ListReleases( + context.Context, string, string, *github.ListOptions, + ) ([]*github.RepositoryRelease, *github.Response, error) } // New creates a new default GitHub client @@ -56,6 +60,12 @@ func (g *githubClient) ListTags( return g.Client.Repositories.ListTags(ctx, owner, repo, opt) } +func (g *githubClient) ListReleases( + ctx context.Context, owner, repo string, opt *github.ListOptions, +) ([]*github.RepositoryRelease, *github.Response, error) { + return g.Client.Repositories.ListReleases(ctx, owner, repo, opt) +} + // SetClient can be used to manually set the internal GitHub client func (g *GitHub) SetClient(client Client) { g.client = client @@ -79,7 +89,7 @@ func (g *GitHub) LatestGitHubTagsPerBranch() (TagsPerBranch, error) { context.Background(), git.DefaultGithubOrg, git.DefaultGithubRepo, nil, ) if err != nil { - return nil, errors.Wrap(err, "unable to retrieve GitHub releases") + return nil, errors.Wrap(err, "unable to retrieve GitHub tags") } releases := make(TagsPerBranch) @@ -117,3 +127,28 @@ func (t TagsPerBranch) addIfNotExisting(branch, tag string) { t[branch] = tag } } + +// Releases returns a list of GitHub releases for the provided `owner` and +// `repo`. If `includePrereleases` is `true`, then the resulting slice will +// also contain pre/drafted releases. +func (g *GitHub) Releases(owner, repo string, includePrereleases bool) ([]*github.RepositoryRelease, error) { + allReleases, _, err := g.client.ListReleases( + context.Background(), owner, repo, nil, + ) + if err != nil { + return nil, errors.Wrap(err, "unable to retrieve GitHub releases") + } + + releases := []*github.RepositoryRelease{} + for _, release := range allReleases { + if release.GetPrerelease() { + if includePrereleases { + releases = append(releases, release) + } + } else { + releases = append(releases, release) + } + } + + return releases, nil +} diff --git a/pkg/github/github_test.go b/pkg/github/github_test.go index 0eddbe15f87..6bcb9931b23 100644 --- a/pkg/github/github_test.go +++ b/pkg/github/github_test.go @@ -164,3 +164,86 @@ func TestLatestGitHubTagsPerBranchFailedNonSemverTag(t *testing.T) { require.NotNil(t, err) require.Nil(t, res) } + +func TestReleasesSuccessEmpty(t *testing.T) { + // Given + sut, client := newSUT() + client.ListReleasesReturns([]*gogithub.RepositoryRelease{}, nil, nil) + + // When + res, err := sut.Releases("", "", false) + + // Then + require.Nil(t, err) + require.Empty(t, res) +} + +func TestReleasesSuccessNoPreReleases(t *testing.T) { + // Given + var ( + tag1 = "v1.18.0" + tag2 = "v1.17.0" + tag3 = "v1.16.0" + tag4 = "v1.15.0" + aTrue = true + ) + sut, client := newSUT() + client.ListReleasesReturns([]*gogithub.RepositoryRelease{ + {TagName: &tag1}, + {TagName: &tag2}, + {TagName: &tag3, Prerelease: &aTrue}, + {TagName: &tag4}, + }, nil, nil) + + // When + res, err := sut.Releases("", "", false) + + // Then + require.Nil(t, err) + require.Len(t, res, 3) + require.Equal(t, tag1, res[0].GetTagName()) + require.Equal(t, tag2, res[1].GetTagName()) + require.Equal(t, tag4, res[2].GetTagName()) +} + +func TestReleasesSuccessWithPreReleases(t *testing.T) { + // Given + var ( + tag1 = "v1.18.0" + tag2 = "v1.17.0" + tag3 = "v1.16.0" + tag4 = "v1.15.0" + aTrue = true + ) + sut, client := newSUT() + client.ListReleasesReturns([]*gogithub.RepositoryRelease{ + {TagName: &tag1}, + {TagName: &tag2, Prerelease: &aTrue}, + {TagName: &tag3, Prerelease: &aTrue}, + {TagName: &tag4}, + }, nil, nil) + + // When + res, err := sut.Releases("", "", true) + + // Then + require.Nil(t, err) + require.Len(t, res, 4) + require.Equal(t, tag1, res[0].GetTagName()) + require.Equal(t, tag2, res[1].GetTagName()) + require.Equal(t, tag3, res[2].GetTagName()) + require.Equal(t, tag4, res[3].GetTagName()) +} + +func TestReleasesFailed(t *testing.T) { + // Given + sut, client := newSUT() + client.ListReleasesReturns(nil, nil, errors.New("error")) + + // When + res, err := sut.Releases("", "", false) + + // Then + require.NotNil(t, err) + require.Nil(t, res, nil) +} diff --git a/pkg/github/githubfakes/fake_client.go b/pkg/github/githubfakes/fake_client.go index c5c280a12ea..0271787ac9d 100644 --- a/pkg/github/githubfakes/fake_client.go +++ b/pkg/github/githubfakes/fake_client.go @@ -26,6 +26,24 @@ import ( ) type FakeClient struct { + ListReleasesStub func(context.Context, string, string, *githuba.ListOptions) ([]*githuba.RepositoryRelease, *githuba.Response, error) + listReleasesMutex sync.RWMutex + listReleasesArgsForCall []struct { + arg1 context.Context + arg2 string + arg3 string + arg4 *githuba.ListOptions + } + listReleasesReturns struct { + result1 []*githuba.RepositoryRelease + result2 *githuba.Response + result3 error + } + listReleasesReturnsOnCall map[int]struct { + result1 []*githuba.RepositoryRelease + result2 *githuba.Response + result3 error + } ListTagsStub func(context.Context, string, string, *githuba.ListOptions) ([]*githuba.RepositoryTag, *githuba.Response, error) listTagsMutex sync.RWMutex listTagsArgsForCall []struct { @@ -48,6 +66,75 @@ type FakeClient struct { invocationsMutex sync.RWMutex } +func (fake *FakeClient) ListReleases(arg1 context.Context, arg2 string, arg3 string, arg4 *githuba.ListOptions) ([]*githuba.RepositoryRelease, *githuba.Response, error) { + fake.listReleasesMutex.Lock() + ret, specificReturn := fake.listReleasesReturnsOnCall[len(fake.listReleasesArgsForCall)] + fake.listReleasesArgsForCall = append(fake.listReleasesArgsForCall, struct { + arg1 context.Context + arg2 string + arg3 string + arg4 *githuba.ListOptions + }{arg1, arg2, arg3, arg4}) + fake.recordInvocation("ListReleases", []interface{}{arg1, arg2, arg3, arg4}) + fake.listReleasesMutex.Unlock() + if fake.ListReleasesStub != nil { + return fake.ListReleasesStub(arg1, arg2, arg3, arg4) + } + if specificReturn { + return ret.result1, ret.result2, ret.result3 + } + fakeReturns := fake.listReleasesReturns + return fakeReturns.result1, fakeReturns.result2, fakeReturns.result3 +} + +func (fake *FakeClient) ListReleasesCallCount() int { + fake.listReleasesMutex.RLock() + defer fake.listReleasesMutex.RUnlock() + return len(fake.listReleasesArgsForCall) +} + +func (fake *FakeClient) ListReleasesCalls(stub func(context.Context, string, string, *githuba.ListOptions) ([]*githuba.RepositoryRelease, *githuba.Response, error)) { + fake.listReleasesMutex.Lock() + defer fake.listReleasesMutex.Unlock() + fake.ListReleasesStub = stub +} + +func (fake *FakeClient) ListReleasesArgsForCall(i int) (context.Context, string, string, *githuba.ListOptions) { + fake.listReleasesMutex.RLock() + defer fake.listReleasesMutex.RUnlock() + argsForCall := fake.listReleasesArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3, argsForCall.arg4 +} + +func (fake *FakeClient) ListReleasesReturns(result1 []*githuba.RepositoryRelease, result2 *githuba.Response, result3 error) { + fake.listReleasesMutex.Lock() + defer fake.listReleasesMutex.Unlock() + fake.ListReleasesStub = nil + fake.listReleasesReturns = struct { + result1 []*githuba.RepositoryRelease + result2 *githuba.Response + result3 error + }{result1, result2, result3} +} + +func (fake *FakeClient) ListReleasesReturnsOnCall(i int, result1 []*githuba.RepositoryRelease, result2 *githuba.Response, result3 error) { + fake.listReleasesMutex.Lock() + defer fake.listReleasesMutex.Unlock() + fake.ListReleasesStub = nil + if fake.listReleasesReturnsOnCall == nil { + fake.listReleasesReturnsOnCall = make(map[int]struct { + result1 []*githuba.RepositoryRelease + result2 *githuba.Response + result3 error + }) + } + fake.listReleasesReturnsOnCall[i] = struct { + result1 []*githuba.RepositoryRelease + result2 *githuba.Response + result3 error + }{result1, result2, result3} +} + func (fake *FakeClient) ListTags(arg1 context.Context, arg2 string, arg3 string, arg4 *githuba.ListOptions) ([]*githuba.RepositoryTag, *githuba.Response, error) { fake.listTagsMutex.Lock() ret, specificReturn := fake.listTagsReturnsOnCall[len(fake.listTagsArgsForCall)] @@ -120,6 +207,8 @@ func (fake *FakeClient) ListTagsReturnsOnCall(i int, result1 []*githuba.Reposito func (fake *FakeClient) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() + fake.listReleasesMutex.RLock() + defer fake.listReleasesMutex.RUnlock() fake.listTagsMutex.RLock() defer fake.listTagsMutex.RUnlock() copiedInvocations := map[string][][]interface{}{} diff --git a/pkg/kubepkg/BUILD.bazel b/pkg/kubepkg/BUILD.bazel index 375f4ce97cc..2d15da1acd6 100644 --- a/pkg/kubepkg/BUILD.bazel +++ b/pkg/kubepkg/BUILD.bazel @@ -10,10 +10,10 @@ go_library( visibility = ["//visibility:public"], deps = [ "//pkg/command:go_default_library", + "//pkg/github:go_default_library", "//pkg/release:go_default_library", "//pkg/util:go_default_library", "@com_github_blang_semver//:go_default_library", - "@com_github_google_go_github_v29//github:go_default_library", "@com_github_pkg_errors//:go_default_library", "@com_github_sirupsen_logrus//:go_default_library", ], diff --git a/pkg/kubepkg/kubepkg.go b/pkg/kubepkg/kubepkg.go index 61148103752..3143519da5e 100644 --- a/pkg/kubepkg/kubepkg.go +++ b/pkg/kubepkg/kubepkg.go @@ -17,7 +17,6 @@ limitations under the License. package kubepkg import ( - "context" "fmt" "io/ioutil" "os" @@ -28,11 +27,11 @@ import ( "time" "github.com/blang/semver" - "github.com/google/go-github/v29/github" "github.com/pkg/errors" "github.com/sirupsen/logrus" "k8s.io/release/pkg/command" + "k8s.io/release/pkg/github" "k8s.io/release/pkg/release" "k8s.io/release/pkg/util" ) @@ -474,7 +473,7 @@ func getCRIToolsVersion(packageDef *PackageDefinition) (string, error) { criToolsVersion := fmt.Sprintf("%s.%s.0", criToolsMajor, criToolsMinor) - releases, err := fetchReleases("kubernetes-sigs", "cri-tools", false) + releases, err := github.New().Releases("kubernetes-sigs", "cri-tools", false) if err != nil { return "", err } @@ -510,28 +509,6 @@ func getCRIToolsVersion(packageDef *PackageDefinition) (string, error) { return criToolsVersion, nil } -func fetchReleases(owner, repo string, includePrereleases bool) ([]*github.RepositoryRelease, error) { - ghClient := github.NewClient(nil) - - allReleases, _, err := ghClient.Repositories.ListReleases(context.Background(), owner, repo, nil) - if err != nil { - return nil, err - } - - var releases []*github.RepositoryRelease - for _, release := range allReleases { - if *release.Prerelease { - if includePrereleases { - releases = append(releases, release) - } - } else { - releases = append(releases, release) - } - } - - return releases, nil -} - func getDownloadLinkBase(packageDef *PackageDefinition) (string, error) { if packageDef == nil { return "", errors.New("package definition cannot be nil")