From 5688b1e8b75572da701664049db1491640029250 Mon Sep 17 00:00:00 2001 From: Ajita Jain Date: Wed, 16 Oct 2024 16:14:38 -0700 Subject: [PATCH] Remove github_host var, rename github_token to github_access_token, match the release sources to the release by org --- .../workflows/create-debugging-artifact.yml | 1 + .github/workflows/release.yml | 4 ++ .github/workflows/test.yml | 1 + .../generating_release_notes.feature | 2 +- .../workflows/scenario/step_funcs_github.go | 2 +- .../workflows/testdata/tiles/v1/Kilnfile | 2 +- .../workflows/testdata/tiles/v2/Kilnfile | 2 +- internal/commands/release_notes.go | 71 +++++++++---------- internal/commands/release_notes_test.go | 47 +++++------- internal/component/github_release_source.go | 2 +- internal/gh/client.go | 26 +++++-- internal/gh/client_test.go | 16 +++-- pkg/cargo/bump.go | 57 +++++++++------ pkg/cargo/bump_test.go | 6 +- pkg/cargo/kilnfile.go | 19 ----- pkg/notes/notes_data.go | 11 +-- pkg/notes/notes_page_test.go | 10 +-- 17 files changed, 146 insertions(+), 133 deletions(-) diff --git a/.github/workflows/create-debugging-artifact.yml b/.github/workflows/create-debugging-artifact.yml index ca3c0c47..f95e553d 100644 --- a/.github/workflows/create-debugging-artifact.yml +++ b/.github/workflows/create-debugging-artifact.yml @@ -41,6 +41,7 @@ jobs: RELEEN_GITHUB_TOKEN: ${{ secrets.RELEEN_GITHUB_TOKEN }} run: | export GITHUB_TOKEN="${RELEEN_GITHUB_TOKEN}" + export GITHUB_ACCESS_TOKEN="${RELEEN_GITHUB_TOKEN}" go test ./... - name: Acceptance Tests diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 434a55b5..e084ff40 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -38,6 +38,7 @@ jobs: RELEEN_GITHUB_TOKEN: ${{ secrets.RELEEN_GITHUB_TOKEN }} run: | export GITHUB_TOKEN="${RELEEN_GITHUB_TOKEN}" + export GITHUB_ACCESS_TOKEN="${RELEEN_GITHUB_TOKEN}" go test --covermode=atomic --coverprofile=kiln-${{github.sha}}-unit-test-code-coverage.out ./... - name: Archive Unit Test Code Coverage Output @@ -53,6 +54,7 @@ jobs: run: | set -euo pipefail export GITHUB_TOKEN="${RELEEN_GITHUB_TOKEN}" + export GITHUB_ACCESS_TOKEN="${RELEEN_GITHUB_TOKEN}" set -x go test -v --timeout 24h --tags acceptance github.com/pivotal-cf/kiln/internal/acceptance/workflows @@ -80,3 +82,5 @@ jobs: args: release --rm-dist env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GITHUB_ACCESS_TOKEN: ${{ secrets.GITHUB_TOKEN }} + diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index fe685da5..3fde5151 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -51,6 +51,7 @@ jobs: run: | set -euo pipefail export GITHUB_TOKEN="${RELEEN_GITHUB_TOKEN}" + export GITHUB_ACCESS_TOKEN="${RELEEN_GITHUB_TOKEN}" set -x go test -v --timeout 15m --tags acceptance github.com/pivotal-cf/kiln/internal/acceptance/workflows diff --git a/internal/acceptance/workflows/generating_release_notes.feature b/internal/acceptance/workflows/generating_release_notes.feature index 52f26588..798b3e43 100644 --- a/internal/acceptance/workflows/generating_release_notes.feature +++ b/internal/acceptance/workflows/generating_release_notes.feature @@ -10,7 +10,7 @@ Feature: As a robot, I want to generate release notes | --name=hello-release | | --version=v0.1.5 | | --without-download | - | --variable=github_token="${GITHUB_TOKEN}" | + | --variable=github_access_token="${GITHUB_TOKEN}" | And I write file "version" | 0.1.4 | And I execute git add Kilnfile.lock version diff --git a/internal/acceptance/workflows/scenario/step_funcs_github.go b/internal/acceptance/workflows/scenario/step_funcs_github.go index e68dae33..6d2b9cdc 100644 --- a/internal/acceptance/workflows/scenario/step_funcs_github.go +++ b/internal/acceptance/workflows/scenario/step_funcs_github.go @@ -13,7 +13,7 @@ func githubRepoHasReleaseWithTag(ctx context.Context, repoOrg, repoName, tag str if err != nil { return err } - ghAPI, err := gh.Client(ctx, "", accessToken) + ghAPI, err := gh.Client(ctx, "", accessToken, accessToken) if err != nil { return fmt.Errorf("failed to setup github client: %w", err) } diff --git a/internal/acceptance/workflows/testdata/tiles/v1/Kilnfile b/internal/acceptance/workflows/testdata/tiles/v1/Kilnfile index f606a540..701818e7 100644 --- a/internal/acceptance/workflows/testdata/tiles/v1/Kilnfile +++ b/internal/acceptance/workflows/testdata/tiles/v1/Kilnfile @@ -3,7 +3,7 @@ slug: crhntr-hello release_sources: - type: github org: crhntr - github_token: $(variable "github_token") + github_token: $(variable "github_access_token") - type: bosh.io releases: - name: bpm diff --git a/internal/acceptance/workflows/testdata/tiles/v2/Kilnfile b/internal/acceptance/workflows/testdata/tiles/v2/Kilnfile index bfe46231..381087e3 100644 --- a/internal/acceptance/workflows/testdata/tiles/v2/Kilnfile +++ b/internal/acceptance/workflows/testdata/tiles/v2/Kilnfile @@ -3,7 +3,7 @@ slug: crhntr-hello release_sources: - type: github org: crhntr - github_token: $(variable "github_token") + github_token: $(variable "github_access_token") - type: bosh.io releases: - name: bpm diff --git a/internal/commands/release_notes.go b/internal/commands/release_notes.go index 679abeec..3e13a8ee 100644 --- a/internal/commands/release_notes.go +++ b/internal/commands/release_notes.go @@ -28,15 +28,15 @@ const releaseDateFormat = "2006-01-02" type ReleaseNotes struct { Options struct { - ReleaseDate string `long:"release-date" short:"d" description:"release date of the tile"` - TemplateName string `long:"template" short:"t" description:"path to template"` - GithubIssuesServiceToken string `long:"github-token" short:"g" description:"auth token for fetching issues and milestones with labels" env:"GITHUB_TOKEN"` - GithubHost string `long:"github-host" description:"set this when you are using GitHub enterprise to fetch issues, milestones or notes for bosh releases" env:"GITHUB_HOST"` - Kilnfile string `long:"kilnfile" short:"k" description:"path to Kilnfile"` - DocsFile string `long:"update-docs" short:"u" description:"path to docs file to update"` - Window string `long:"window" short:"w" description:"GA window for release notes" default:"ga"` - VariableFiles []string `long:"variables-file" short:"vf" description:"path to a file containing variables to interpolate"` - Variables []string `long:"variable" short:"vr" description:"key value pairs of variables to interpolate"` + ReleaseDate string `long:"release-date" short:"d" description:"release date of the tile"` + TemplateName string `long:"template" short:"t" description:"path to template"` + GithubAccessToken string `long:"github_access_token" short:"g" description:"auth token for github.com" env:"GITHUB_ACCESS_TOKEN"` + GithubEnterpriseAccessToken string `long:"github_enterprise_access_token" short:"ge" description:"auth token for github enterprise" env:"GITHUB_ENTERPRISE_ACCESS_TOKEN"` + Kilnfile string `long:"kilnfile" short:"k" description:"path to Kilnfile"` + DocsFile string `long:"update-docs" short:"u" description:"path to docs file to update"` + Window string `long:"window" short:"w" description:"GA window for release notes" default:"ga"` + VariableFiles []string `long:"variables-file" short:"vf" description:"path to a file containing variables to interpolate"` + Variables []string `long:"variable" short:"vr" description:"key value pairs of variables to interpolate"` notes.IssuesQuery notes.TrainstatQuery } @@ -49,10 +49,10 @@ type ReleaseNotes struct { fetchNotesData FetchNotesData variablesService baking.TemplateVariablesService - repoOwner, repoName string + repoHost, repoOwner, repoName string } -type FetchNotesData func(ctx context.Context, repo *git.Repository, client *github.Client, tileRepoOwner, tileRepoName, kilnfilePath, initialRevision, finalRevision string, issuesQuery notes.IssuesQuery, trainstatClient notes.TrainstatNotesFetcher, variables map[string]any) (notes.Data, error) +type FetchNotesData func(ctx context.Context, repo *git.Repository, client *github.Client, tileRepoHost, tileRepoOwner, tileRepoName, kilnfilePath, initialRevision, finalRevision string, issuesQuery notes.IssuesQuery, trainstatClient notes.TrainstatNotesFetcher, variables map[string]any) (notes.Data, error) func NewReleaseNotesCommand() (ReleaseNotes, error) { return ReleaseNotes{ @@ -82,16 +82,16 @@ func (r ReleaseNotes) Execute(args []string) error { if err != nil { return fmt.Errorf("failed to parse template variables: %s", err) } - if varValue, ok := templateVariables["github_token"]; !ok && r.Options.GithubIssuesServiceToken != "" { - templateVariables["github_token"] = r.Options.GithubIssuesServiceToken - } else if ok && r.Options.GithubIssuesServiceToken == "" { - r.Options.GithubIssuesServiceToken = varValue.(string) + if varValue, ok := templateVariables["github_access_token"]; !ok && r.Options.GithubAccessToken != "" { + templateVariables["github_access_token"] = r.Options.GithubAccessToken + } else if ok && r.Options.GithubAccessToken == "" { + r.Options.GithubAccessToken = varValue.(string) } - if varValue, ok := templateVariables["github_host"]; !ok && r.Options.GithubHost != "" { - templateVariables["github_host"] = r.Options.GithubHost - } else if ok && r.Options.GithubHost == "" { - r.Options.GithubHost = varValue.(string) + if varValue, ok := templateVariables["github_enterprise_access_token"]; !ok && r.Options.GithubAccessToken != "" { + templateVariables["github_enterprise_access_token"] = r.Options.GithubEnterpriseAccessToken + } else if ok && r.Options.GithubEnterpriseAccessToken == "" { + r.Options.GithubEnterpriseAccessToken = varValue.(string) } ctx := context.Background() @@ -106,18 +106,17 @@ func (r ReleaseNotes) Execute(args []string) error { } var client *github.Client - if r.Options.GithubIssuesServiceToken != "" { - client, err = gh.Client(ctx, r.Options.GithubHost, r.Options.GithubIssuesServiceToken) - if err != nil { - return fmt.Errorf("failed to setup github client: %w", err) - } + + client, err = gh.Client(ctx, r.repoHost, r.Options.GithubAccessToken, r.Options.GithubEnterpriseAccessToken) + if err != nil { + return fmt.Errorf("failed to setup github client: %w", err) } trainstatClient := notes.NewTrainstatClient(r.Options.TrainstatQuery.TrainstatURL) _ = notes.FetchData // fetchNotesData is github.com/pivotal/kiln/internal/notes.FetchData data, err := r.fetchNotesData(ctx, - r.repository, client, r.repoOwner, r.repoName, + r.repository, client, r.repoHost, r.repoOwner, r.repoName, r.Options.Kilnfile, nonFlagArgs[0], nonFlagArgs[1], r.Options.IssuesQuery, @@ -196,12 +195,13 @@ func (r *ReleaseNotes) initRepo() error { return fmt.Errorf("release-notes must be run from the root of the repository (use --kilnfile flag to specify which tile to build)") } - repoOwner, repoName, err := getGithubRemoteRepoOwnerAndName(repo) + repoHost, repoOwner, repoName, err := getGithubRemoteHostRepoOwnerAndName(repo) if err != nil { return err } r.repository = repo + r.repoHost = repoHost r.repoName = repoName r.repoOwner = repoOwner @@ -243,11 +243,8 @@ func (r ReleaseNotes) checkInputs(nonFlagArgs []string) error { } } - if r.Options.GithubIssuesServiceToken == "" && - (r.Options.IssueMilestone != "" || - len(r.Options.IssueIDs) > 0 || - len(r.Options.IssueLabels) > 0) { - return errors.New("github-token (env: GITHUB_TOKEN) must be set to interact with the github api") + if r.Options.GithubEnterpriseAccessToken == "" && r.Options.GithubAccessToken == "" { + return errors.New("github_access_token(env: GITHUB_ACCESS_TOKEN) and/or github_enterprise_access_token(env: GITHUB_ENTERPRISE_ACCESS_TOKEN) must be set to interact with the github api") } if r.Options.DocsFile != "" { @@ -279,11 +276,11 @@ func (r ReleaseNotes) parseReleaseDate() (time.Time, error) { return releaseDate, nil } -func getGithubRemoteRepoOwnerAndName(repo *git.Repository) (string, string, error) { +func getGithubRemoteHostRepoOwnerAndName(repo *git.Repository) (string, string, string, error) { var remoteURL string remote, err := repo.Remote("origin") if err != nil { - return "", "", err + return "", "", "", err } config := remote.Config() for _, u := range config.URLs { @@ -291,13 +288,13 @@ func getGithubRemoteRepoOwnerAndName(repo *git.Repository) (string, string, erro break } if remoteURL == "" { - return "", "", fmt.Errorf("remote github URL not found for repo") + return "", "", "", fmt.Errorf("remote github URL not found for repo") } - repoOwner, repoName, err := gh.RepositoryOwnerAndNameFromPath(remoteURL) + repoHost, repoOwner, repoName, err := gh.RepositoryHostOwnerAndNameFromPath(remoteURL) if err != nil { - return "", "", err + return "", "", "", err } - return repoOwner, repoName, nil + return repoHost, repoOwner, repoName, nil } diff --git a/internal/commands/release_notes_test.go b/internal/commands/release_notes_test.go index 948f898c..91bfbfe7 100644 --- a/internal/commands/release_notes_test.go +++ b/internal/commands/release_notes_test.go @@ -57,7 +57,7 @@ func TestReleaseNotes_Execute(t *testing.T) { } var ( - tileRepoOwner, tileRepoName, kilnfilePath, initialRevision, finalRevision string + tileRepoHost, tileRepoOwner, tileRepoName, kilnfilePath, initialRevision, finalRevision string issuesQuery notes.IssuesQuery repository *git.Repository @@ -69,12 +69,13 @@ func TestReleaseNotes_Execute(t *testing.T) { rn := ReleaseNotes{ Writer: &out, repository: nonNilRepo, + repoHost: "github.com", repoOwner: "bunch", repoName: "banana", readFile: readFileFunc, - fetchNotesData: func(c context.Context, repo *git.Repository, ghc *github.Client, tro, trn, kfp, ir, fr string, iq notes.IssuesQuery, _ notes.TrainstatNotesFetcher, __ map[string]any) (notes.Data, error) { + fetchNotesData: func(c context.Context, repo *git.Repository, ghc *github.Client, trh, tro, trn, kfp, ir, fr string, iq notes.IssuesQuery, _ notes.TrainstatNotesFetcher, __ map[string]any) (notes.Data, error) { ctx, repository, client = c, repo, ghc - tileRepoOwner, tileRepoName, kilnfilePath, initialRevision, finalRevision = tro, trn, kfp, ir, fr + tileRepoHost, tileRepoOwner, tileRepoName, kilnfilePath, initialRevision, finalRevision = trh, tro, trn, kfp, ir, fr issuesQuery = iq return notes.Data{ ReleaseDate: mustParseTime(time.Parse(releaseDateFormat, "2021-11-04")), @@ -104,12 +105,12 @@ func TestReleaseNotes_Execute(t *testing.T) { }, } - rn.Options.GithubIssuesServiceToken = "secret" + rn.Options.GithubAccessToken = "secret" err := rn.Execute([]string{ "--kilnfile=tile/Kilnfile", "--release-date=2021-11-04", - "--github-token=lemon", + "--github_access_token=lemon", "--github-issue-milestone=smoothie", "--github-issue-label=tropical", "--github-issue=54000", @@ -124,6 +125,7 @@ func TestReleaseNotes_Execute(t *testing.T) { please.Expect(repository).NotTo(BeNil()) please.Expect(client).NotTo(BeNil()) + please.Expect(tileRepoHost).To(Equal("github.com")) please.Expect(tileRepoOwner).To(Equal("bunch")) please.Expect(tileRepoName).To(Equal("banana")) please.Expect(kilnfilePath).To(Equal("tile/Kilnfile")) @@ -188,6 +190,7 @@ func TestReleaseNotes_checkInputs(t *testing.T) { rn := ReleaseNotes{} rn.Options.ReleaseDate = `some-date` + rn.Options.GithubAccessToken = "test-token" err := rn.checkInputs([]string{"a", "b"}) please.Expect(err).To(MatchError(ContainSubstring("cannot parse"))) }) @@ -197,27 +200,9 @@ func TestReleaseNotes_checkInputs(t *testing.T) { please := NewWithT(t) rn := ReleaseNotes{} - rn.Options.IssueMilestone = "s" err := rn.checkInputs([]string{"a", "b"}) - please.Expect(err).To(MatchError(ContainSubstring("github-token"))) - }) - - t.Run("ids", func(t *testing.T) { - please := NewWithT(t) - - rn := ReleaseNotes{} - rn.Options.IssueIDs = []string{"s"} - err := rn.checkInputs([]string{"a", "b"}) - please.Expect(err).To(MatchError(ContainSubstring("github-token"))) - }) - - t.Run("labels", func(t *testing.T) { - please := NewWithT(t) - - rn := ReleaseNotes{} - rn.Options.IssueLabels = []string{"s"} - err := rn.checkInputs([]string{"a", "b"}) - please.Expect(err).To(MatchError(ContainSubstring("github-token"))) + please.Expect(err).To(MatchError(ContainSubstring("github_access_token"))) + please.Expect(err).To(MatchError(ContainSubstring("github_enterprise_access_token"))) }) t.Run("exp", func(t *testing.T) { @@ -225,6 +210,7 @@ func TestReleaseNotes_checkInputs(t *testing.T) { rn := ReleaseNotes{} rn.Options.IssueTitleExp = "s" + rn.Options.GithubEnterpriseAccessToken = "test-token" err := rn.checkInputs([]string{"a", "b"}) please.Expect(err).NotTo(HaveOccurred()) }) @@ -243,8 +229,9 @@ func Test_getGithubRemoteRepoOwnerAndName(t *testing.T) { "https://github.com/pivotal-cf/kiln", }, }) - o, r, err := getGithubRemoteRepoOwnerAndName(repo) + h, o, r, err := getGithubRemoteHostRepoOwnerAndName(repo) please.Expect(err).NotTo(HaveOccurred()) + please.Expect(h).To(Equal("github.com")) please.Expect(o).To(Equal("pivotal-cf")) please.Expect(r).To(Equal("kiln")) }) @@ -259,8 +246,9 @@ func Test_getGithubRemoteRepoOwnerAndName(t *testing.T) { "git@github.com:pivotal-cf/kiln.git", }, }) - o, r, err := getGithubRemoteRepoOwnerAndName(repo) + h, o, r, err := getGithubRemoteHostRepoOwnerAndName(repo) please.Expect(err).NotTo(HaveOccurred()) + please.Expect(h).To(Equal("github.com")) please.Expect(o).To(Equal("pivotal-cf")) please.Expect(r).To(Equal("kiln")) }) @@ -269,7 +257,7 @@ func Test_getGithubRemoteRepoOwnerAndName(t *testing.T) { please := NewWithT(t) repo, _ := git.Init(memory.NewStorage(), memfs.New()) - _, _, err := getGithubRemoteRepoOwnerAndName(repo) + _, _, _, err := getGithubRemoteHostRepoOwnerAndName(repo) please.Expect(err).To(MatchError(ContainSubstring("not found"))) }) @@ -289,8 +277,9 @@ func Test_getGithubRemoteRepoOwnerAndName(t *testing.T) { "git@github.com:pivotal-cf/kiln.git", }, }) - o, _, err := getGithubRemoteRepoOwnerAndName(repo) + h, o, _, err := getGithubRemoteHostRepoOwnerAndName(repo) please.Expect(err).NotTo(HaveOccurred()) + please.Expect(h).To(Equal("github.com")) please.Expect(o).To(Equal("pivotal-cf"), "it uses the remote with name 'origin'") }) } diff --git a/internal/component/github_release_source.go b/internal/component/github_release_source.go index 0c7ab317..759d42a8 100644 --- a/internal/component/github_release_source.go +++ b/internal/component/github_release_source.go @@ -43,7 +43,7 @@ func NewGithubReleaseSource(c cargo.ReleaseSourceConfig) *GithubReleaseSource { if c.Org == "" { panic("no github org passed for github release source") } - githubClient, err := c.GitHubClient(context.TODO()) + githubClient, err := gh.Client(context.TODO(), "", c.GithubToken, c.GithubToken) // host is github.com by default if err != nil { panic(err) } diff --git a/internal/gh/client.go b/internal/gh/client.go index e4db8aef..cea9962a 100644 --- a/internal/gh/client.go +++ b/internal/gh/client.go @@ -2,15 +2,33 @@ package gh import ( "context" + "errors" + "fmt" + "strings" "github.com/google/go-github/v50/github" "golang.org/x/oauth2" ) -func Client(ctx context.Context, host, accessToken string) (*github.Client, error) { - client := oauth2.NewClient(ctx, oauth2.StaticTokenSource(&oauth2.Token{AccessToken: accessToken})) - if host == "" { +/* Client +Creates a GitHub client based on the host. +If host = GitHub Enterprise Host, uses githubEnterpriseAccessToken +Else it assumes host as GitHub.com and uses githubAccessToken +*/ + +func Client(ctx context.Context, host, githubAccessToken string, githubEnterpriseAccessToken string) (*github.Client, error) { + if host != "" && strings.HasSuffix(host, "broadcom.net") { + if githubEnterpriseAccessToken == "" { + return nil, errors.New("github enterprise access token is absent") + } + client := oauth2.NewClient(ctx, oauth2.StaticTokenSource(&oauth2.Token{AccessToken: githubEnterpriseAccessToken})) + return github.NewEnterpriseClient(host, host, client) + } else if host == "" || host == "github.com" { + if githubAccessToken == "" { + return nil, fmt.Errorf("github access token (github.com) is absent") + } + client := oauth2.NewClient(ctx, oauth2.StaticTokenSource(&oauth2.Token{AccessToken: githubAccessToken})) return github.NewClient(client), nil } - return github.NewEnterpriseClient(host, host, client) + return nil, errors.New("github host not recognized") } diff --git a/internal/gh/client_test.go b/internal/gh/client_test.go index bcb97fab..fe7b996a 100644 --- a/internal/gh/client_test.go +++ b/internal/gh/client_test.go @@ -13,18 +13,26 @@ func TestClient(t *testing.T) { t.Run("when the host is empty", func(t *testing.T) { ctx := context.Background() token := "xxx" - ghClient, err := gh.Client(ctx, "", token) + ghClient, err := gh.Client(ctx, "", token, token) require.NoError(t, err) require.NotNil(t, ghClient.Client()) assert.Contains(t, ghClient.BaseURL.String(), "https://api.github.com") }) - t.Run("when the host is not empty", func(t *testing.T) { + t.Run("when the host point to enterprise github", func(t *testing.T) { ctx := context.Background() token := "xxx" - ghClient, err := gh.Client(ctx, "https://example.com", token) + ghClient, err := gh.Client(ctx, "https://broadcom.net", token, token) require.NoError(t, err) require.NotNil(t, ghClient.Client()) - assert.Contains(t, ghClient.BaseURL.String(), "https://example.com") + assert.Contains(t, ghClient.BaseURL.String(), "https://broadcom.net") + }) + + t.Run("when the host point to non-enterprise random github", func(t *testing.T) { + ctx := context.Background() + token := "xxx" + ghClient, err := gh.Client(ctx, "https://example.com", token, token) + require.Error(t, err, "github host not recognized") + require.Nil(t, ghClient) }) } diff --git a/pkg/cargo/bump.go b/pkg/cargo/bump.go index aff36209..0f80bc68 100644 --- a/pkg/cargo/bump.go +++ b/pkg/cargo/bump.go @@ -2,7 +2,6 @@ package cargo import ( "context" - "fmt" "log" "slices" "sort" @@ -129,7 +128,7 @@ type repositoryReleaseLister interface { type ( RepositoryReleaseLister = repositoryReleaseLister - githubReleasesClient func(ctx context.Context, kilnfile Kilnfile, lock BOSHReleaseTarballLock) (repositoryReleaseLister, error) + githubReleasesClient func(ctx context.Context, kilnfile Kilnfile, lock BOSHReleaseTarballLock) ([]repositoryReleaseLister, error) ) // Fetch release notes for each of the release bumps in the Kilnfile @@ -188,9 +187,8 @@ func ReleaseNotes(ctx context.Context, kf Kilnfile, list BumpList) (BumpList, er return releaseNotes(ctx, kf, list, getGithubRepositoryClientForRelease(kf)) } -func getGithubRepositoryClientForRelease(kf Kilnfile) func(ctx context.Context, _ Kilnfile, lock BOSHReleaseTarballLock) (repositoryReleaseLister, error) { - // todo: []repositoryReleaseLister - return func(ctx context.Context, kilnfile Kilnfile, lock BOSHReleaseTarballLock) (repositoryReleaseLister, error) { +func getGithubRepositoryClientForRelease(kf Kilnfile) func(ctx context.Context, _ Kilnfile, lock BOSHReleaseTarballLock) ([]repositoryReleaseLister, error) { + return func(ctx context.Context, kilnfile Kilnfile, lock BOSHReleaseTarballLock) ([]repositoryReleaseLister, error) { spec, err := kf.BOSHReleaseTarballSpecification(lock.Name) if err != nil { return nil, err @@ -201,24 +199,26 @@ func getGithubRepositoryClientForRelease(kf Kilnfile) func(ctx context.Context, return nil, err } - i := slices.IndexFunc(kf.ReleaseSources, func(config ReleaseSourceConfig) bool { - return config.Type == BOSHReleaseTarballSourceTypeGithub && config.Org == owner - }) - if i < 0 { - return nil, fmt.Errorf("release source with id %s not found", lock.RemoteSource) - } - source := kf.ReleaseSources[i] + var repoReleaseLister []repositoryReleaseLister + // Create GitHub clients for all the release sources that have same org as the bosh release. Map the client to + // client.Repositories + for _, releaseSourceConfig := range kf.ReleaseSources { + if releaseSourceConfig.Type == BOSHReleaseTarballSourceTypeGithub && releaseSourceConfig.Org == owner { + client, err := gh.Client(ctx, host, releaseSourceConfig.GithubToken, releaseSourceConfig.GithubToken) + if err != nil { + return nil, err + } + repoReleaseLister = append(repoReleaseLister, client.Repositories) - client, err := source.GitHubClient(ctx, host) - if err != nil { - return nil, err + } } - return client.Repositories, err + + return repoReleaseLister, err } } // Fetch all the releases from GitHub repository between from and to versions -func fetchReleasesFromRepo(ctx context.Context, releaseLister RepositoryReleaseLister, repository string, from, to *semver.Version) []*github.RepositoryRelease { +func fetchReleasesFromRepo(ctx context.Context, releaseListers []RepositoryReleaseLister, repository string, from, to *semver.Version) []*github.RepositoryRelease { owner, repo, err := gh.RepositoryOwnerAndNameFromPath(repository) if err != nil { return nil @@ -227,9 +227,18 @@ func fetchReleasesFromRepo(ctx context.Context, releaseLister RepositoryReleaseL var result []*github.RepositoryRelease ops := github.ListOptions{} - releases, _, err := releaseLister.ListReleases(ctx, owner, repo, &ops) - if err != nil { - log.Println(err) + var releases []*github.RepositoryRelease + for _, releaseLister := range releaseListers { + releases, _, err = releaseLister.ListReleases(ctx, owner, repo, &ops) + if err != nil { + log.Println(err) + // We have multiple releaseListers because there can be different release sources in Kilnfile with the same org but diff access token. + // If we are unable to fetch the releases due to Bad Credentials using one lister, we can try another with different access token. + if releases != nil { + break + } + } + } for _, rel := range releases { @@ -254,8 +263,10 @@ func fetchReleasesForBump(ctx context.Context, kf Kilnfile, bump Bump, getGithub return bump } - // Fetch the GitHub releases client for a single release (bump) in the Kilnfile - releaseLister, err := getGithubRepositoryClientForRelease(ctx, kf, bump.To) + // Fetch the GitHub releases clients for a single release (bump) in the Kilnfile. + // There can be multiple clients present for a release because Kilnfile can have different release sources matching the release's org + // Need to try out different clients to figure out the right one based on the access token + releaseListers, err := getGithubRepositoryClientForRelease(ctx, kf, bump.To) if err != nil { log.Println(err) return bump @@ -267,7 +278,7 @@ func fetchReleasesForBump(ctx context.Context, kf Kilnfile, bump Bump, getGithub } if spec.GitHubRepository != "" { - releases := fetchReleasesFromRepo(ctx, releaseLister, spec.GitHubRepository, from, to) + releases := fetchReleasesFromRepo(ctx, releaseListers, spec.GitHubRepository, from, to) bump.Releases = append(bump.Releases, releases...) } diff --git a/pkg/cargo/bump_test.go b/pkg/cargo/bump_test.go index e1ea2d55..4b7d9688 100644 --- a/pkg/cargo/bump_test.go +++ b/pkg/cargo/bump_test.go @@ -164,8 +164,10 @@ func TestInternal_addReleaseNotes(t *testing.T) { To: BOSHReleaseTarballLock{Version: "10"}, From: BOSHReleaseTarballLock{Version: "9"}, }, - }, func(ctx context.Context, kilnfile Kilnfile, lock BOSHReleaseTarballLock) (repositoryReleaseLister, error) { - return releaseLister, nil + }, func(ctx context.Context, kilnfile Kilnfile, lock BOSHReleaseTarballLock) ([]repositoryReleaseLister, error) { + var r []repositoryReleaseLister + r = append(r, releaseLister) + return r, nil }) please.Expect(err).NotTo(HaveOccurred()) please.Expect(result).To(HaveLen(2)) diff --git a/pkg/cargo/kilnfile.go b/pkg/cargo/kilnfile.go index fea2d37f..4622543e 100644 --- a/pkg/cargo/kilnfile.go +++ b/pkg/cargo/kilnfile.go @@ -1,13 +1,10 @@ package cargo import ( - "context" "errors" "fmt" - "github.com/pivotal-cf/kiln/internal/gh" "strings" - "github.com/google/go-github/v50/github" "gopkg.in/yaml.v3" "github.com/Masterminds/semver/v3" @@ -172,22 +169,6 @@ type ReleaseSourceConfig struct { Password string `yaml:"password,omitempty"` } -func (c ReleaseSourceConfig) GitHubClient(ctx context.Context, githubHost string) (*github.Client, error) { - if c.GithubToken == "" { - return nil, errors.New("no token passed for github release source") - } - //tokenSource := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: c.GithubToken}) - //tokenClient := oauth2.NewClient(ctx, tokenSource) - //var githubClient *github.Client - //gh.Client(ctx) todo use gh.Client not github.NewClient or github.NewEnterpriseClient - return gh.Client(ctx, host, c.GithubToken, c.GithubEnterpriseToken) - //if strings.HasSuffix(githubHost, "broadcom.net") { - // return github.NewEnterpriseClient(c.Endpoint, c.Endpoint, tokenClient) - //} - //githubClient = github.NewClient(tokenClient) - //return githubClient, nil -} - // BOSHReleaseTarballLock represents an exact build of a bosh release // It may identify the where the release is cached; // it may identify the stemcell used to compile the release. diff --git a/pkg/notes/notes_data.go b/pkg/notes/notes_data.go index f5fa8f05..de8ee221 100644 --- a/pkg/notes/notes_data.go +++ b/pkg/notes/notes_data.go @@ -127,20 +127,21 @@ func (q IssuesQuery) Exp() (*regexp.Regexp, error) { return regexp.Compile(str) } -func FetchData(ctx context.Context, repo *git.Repository, client *github.Client, tileRepoOwner, tileRepoName, kilnfilePath, initialRevision, finalRevision string, issuesQuery IssuesQuery, trainstatClient TrainstatNotesFetcher, variables map[string]any) (Data, error) { - f, err := newFetchNotesData(repo, tileRepoOwner, tileRepoName, kilnfilePath, initialRevision, finalRevision, client, issuesQuery, trainstatClient, variables) +func FetchData(ctx context.Context, repo *git.Repository, client *github.Client, tileRepoHost, tileRepoOwner, tileRepoName, kilnfilePath, initialRevision, finalRevision string, issuesQuery IssuesQuery, trainstatClient TrainstatNotesFetcher, variables map[string]any) (Data, error) { + f, err := newFetchNotesData(repo, tileRepoHost, tileRepoOwner, tileRepoName, kilnfilePath, initialRevision, finalRevision, client, issuesQuery, trainstatClient, variables) if err != nil { return Data{}, err } return f.fetch(ctx) } -func newFetchNotesData(repo *git.Repository, tileRepoOwner string, tileRepoName string, kilnfilePath string, initialRevision string, finalRevision string, client *github.Client, issuesQuery IssuesQuery, trainstatClient TrainstatNotesFetcher, variables map[string]any) (fetchNotesData, error) { +func newFetchNotesData(repo *git.Repository, tileRepoHost string, tileRepoOwner string, tileRepoName string, kilnfilePath string, initialRevision string, finalRevision string, client *github.Client, issuesQuery IssuesQuery, trainstatClient TrainstatNotesFetcher, variables map[string]any) (fetchNotesData, error) { if repo == nil { return fetchNotesData{}, errors.New("git repository required to generate release notes") } f := fetchNotesData{ + repoHost: tileRepoHost, repoOwner: tileRepoOwner, repoName: tileRepoName, kilnfilePath: kilnfilePath, @@ -174,7 +175,7 @@ type fetchNotesData struct { issuesService - repoOwner, repoName, + repoHost, repoOwner, repoName, kilnfilePath, initialRevision, finalRevision string @@ -329,7 +330,7 @@ type issuesService interface { // manual test to ensure it continues to behave as expected during refactors. // // The function can be tested by generating release notes for a tile with issue ids and a milestone set. The happy path -// test for Execute does not set GithubIssuesServiceToken intentionally so this code is not triggered and Execute does not actually +// test for Execute does not set GithubToken intentionally so this code is not triggered and Execute does not actually // reach out to GitHub. func (r fetchNotesData) fetchIssuesAndReleaseNotes(ctx context.Context, finalKF, wtKF cargo.Kilnfile, bumpList cargo.BumpList, issuesQuery IssuesQuery) ([]*github.Issue, cargo.BumpList, error) { if r.issuesService == nil { diff --git a/pkg/notes/notes_page_test.go b/pkg/notes/notes_page_test.go index c0ebad3f..fb38f508 100644 --- a/pkg/notes/notes_page_test.go +++ b/pkg/notes/notes_page_test.go @@ -204,7 +204,7 @@ func TestParseNotesPageWithExpressionAndReleasesSentinel(t *testing.T) { func Test_newFetchNotesData(t *testing.T) { t.Run("when called", func(t *testing.T) { please := NewWithT(t) - f, err := newFetchNotesData(&git.Repository{}, "o", "r", "k", "ri", "rf", nil, IssuesQuery{ + f, err := newFetchNotesData(&git.Repository{}, "h", "o", "r", "k", "ri", "rf", nil, IssuesQuery{ IssueMilestone: "BLA", }, &TrainstatClient{ host: "test", @@ -226,14 +226,14 @@ func Test_newFetchNotesData(t *testing.T) { }) t.Run("when repo is nil", func(t *testing.T) { please := NewWithT(t) - _, err := newFetchNotesData(nil, "o", "r", "k", "ri", "rf", &github.Client{}, IssuesQuery{}, &TrainstatClient{}, make(map[string]any)) + _, err := newFetchNotesData(nil, "h", "o", "r", "k", "ri", "rf", &github.Client{}, IssuesQuery{}, &TrainstatClient{}, make(map[string]any)) please.Expect(err).To(HaveOccurred()) }) t.Run("when repo is not nil", func(t *testing.T) { please := NewWithT(t) f, err := newFetchNotesData(&git.Repository{ Storer: &memory.Storage{}, - }, "o", "r", "k", "ri", "rf", &github.Client{}, IssuesQuery{}, &TrainstatClient{}, make(map[string]any)) + }, "h", "o", "r", "k", "ri", "rf", &github.Client{}, IssuesQuery{}, &TrainstatClient{}, make(map[string]any)) please.Expect(err).NotTo(HaveOccurred()) please.Expect(f.repository).NotTo(BeNil()) please.Expect(f.revisionResolver).NotTo(BeNil()) @@ -241,7 +241,7 @@ func Test_newFetchNotesData(t *testing.T) { }) t.Run("when github client is not nil", func(t *testing.T) { please := NewWithT(t) - f, err := newFetchNotesData(&git.Repository{}, "o", "r", "k", "ri", "rf", &github.Client{ + f, err := newFetchNotesData(&git.Repository{}, "h", "o", "r", "k", "ri", "rf", &github.Client{ Issues: &github.IssuesService{}, Repositories: &github.RepositoriesService{}, }, IssuesQuery{}, &TrainstatClient{}, make(map[string]any)) @@ -250,7 +250,7 @@ func Test_newFetchNotesData(t *testing.T) { }) t.Run("when github client is nil", func(t *testing.T) { please := NewWithT(t) - f, err := newFetchNotesData(&git.Repository{}, "o", "r", "k", "ri", "rf", nil, IssuesQuery{}, &TrainstatClient{}, make(map[string]any)) + f, err := newFetchNotesData(&git.Repository{}, "h", "o", "r", "k", "ri", "rf", nil, IssuesQuery{}, &TrainstatClient{}, make(map[string]any)) please.Expect(err).NotTo(HaveOccurred()) please.Expect(f.issuesService).To(BeNil()) })