From 5882a6bf2ce1f140709da16ec3aad098781fea97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20T=C3=B6lle?= Date: Sat, 17 Aug 2024 16:26:40 +0200 Subject: [PATCH] refactor: move commit analyzing out of forge --- changelog.go | 16 +++---- changelog_test.go | 38 +++++++-------- cmd/rp/cmd/run.go | 2 +- commits.go | 15 +++++- commits_test.go | 2 +- forge.go | 117 +++++++++++++++++++-------------------------- git.go | 5 -- releasepr.go | 5 +- releaserpleaser.go | 24 ++++++---- versioning.go | 28 +++++------ versioning_test.go | 88 +++++++++++----------------------- 11 files changed, 152 insertions(+), 188 deletions(-) diff --git a/changelog.go b/changelog.go index 40c65d4..016e876 100644 --- a/changelog.go +++ b/changelog.go @@ -90,18 +90,16 @@ func UpdateChangelogFile(wt *git.Worktree, newEntry string) error { return nil } -func NewChangelogEntry(changesets []Changeset, version, link, prefix, suffix string) (string, error) { +func NewChangelogEntry(commits []AnalyzedCommit, version, link, prefix, suffix string) (string, error) { features := make([]AnalyzedCommit, 0) fixes := make([]AnalyzedCommit, 0) - for _, changeset := range changesets { - for _, commit := range changeset.ChangelogEntries { - switch commit.Type { - case "feat": - features = append(features, commit) - case "fix": - fixes = append(fixes, commit) - } + for _, commit := range commits { + switch commit.Type { + case "feat": + features = append(features, commit) + case "fix": + fixes = append(fixes, commit) } } diff --git a/changelog_test.go b/changelog_test.go index 91ffc85..3d1612b 100644 --- a/changelog_test.go +++ b/changelog_test.go @@ -96,11 +96,11 @@ func TestUpdateChangelogFile(t *testing.T) { func Test_NewChangelogEntry(t *testing.T) { type args struct { - changesets []Changeset - version string - link string - prefix string - suffix string + analyzedCommits []AnalyzedCommit + version string + link string + prefix string + suffix string } tests := []struct { name string @@ -111,9 +111,9 @@ func Test_NewChangelogEntry(t *testing.T) { { name: "empty", args: args{ - changesets: []Changeset{{ChangelogEntries: []AnalyzedCommit{}}}, - version: "1.0.0", - link: "https://example.com/1.0.0", + analyzedCommits: []AnalyzedCommit{}, + version: "1.0.0", + link: "https://example.com/1.0.0", }, want: "## [1.0.0](https://example.com/1.0.0)", wantErr: assert.NoError, @@ -121,13 +121,13 @@ func Test_NewChangelogEntry(t *testing.T) { { name: "single feature", args: args{ - changesets: []Changeset{{ChangelogEntries: []AnalyzedCommit{ + analyzedCommits: []AnalyzedCommit{ { Commit: Commit{}, Type: "feat", Description: "Foobar!", }, - }}}, + }, version: "1.0.0", link: "https://example.com/1.0.0", }, @@ -137,13 +137,13 @@ func Test_NewChangelogEntry(t *testing.T) { { name: "single fix", args: args{ - changesets: []Changeset{{ChangelogEntries: []AnalyzedCommit{ + analyzedCommits: []AnalyzedCommit{ { Commit: Commit{}, Type: "fix", Description: "Foobar!", }, - }}}, + }, version: "1.0.0", link: "https://example.com/1.0.0", }, @@ -153,7 +153,7 @@ func Test_NewChangelogEntry(t *testing.T) { { name: "multiple commits with scopes", args: args{ - changesets: []Changeset{{ChangelogEntries: []AnalyzedCommit{ + analyzedCommits: []AnalyzedCommit{ { Commit: Commit{}, Type: "feat", @@ -176,7 +176,7 @@ func Test_NewChangelogEntry(t *testing.T) { Description: "So sad!", Scope: ptr("sad"), }, - }}}, + }, version: "1.0.0", link: "https://example.com/1.0.0", }, @@ -196,13 +196,13 @@ func Test_NewChangelogEntry(t *testing.T) { { name: "prefix", args: args{ - changesets: []Changeset{{ChangelogEntries: []AnalyzedCommit{ + analyzedCommits: []AnalyzedCommit{ { Commit: Commit{}, Type: "fix", Description: "Foobar!", }, - }}}, + }, version: "1.0.0", link: "https://example.com/1.0.0", prefix: "### Breaking Changes", @@ -219,13 +219,13 @@ func Test_NewChangelogEntry(t *testing.T) { { name: "suffix", args: args{ - changesets: []Changeset{{ChangelogEntries: []AnalyzedCommit{ + analyzedCommits: []AnalyzedCommit{ { Commit: Commit{}, Type: "fix", Description: "Foobar!", }, - }}}, + }, version: "1.0.0", link: "https://example.com/1.0.0", suffix: "### Compatibility\n\nThis version is compatible with flux-compensator v2.2 - v2.9.", @@ -245,7 +245,7 @@ This version is compatible with flux-compensator v2.2 - v2.9. for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := NewChangelogEntry(tt.args.changesets, tt.args.version, tt.args.link, tt.args.prefix, tt.args.suffix) + got, err := NewChangelogEntry(tt.args.analyzedCommits, tt.args.version, tt.args.link, tt.args.prefix, tt.args.suffix) if !tt.wantErr(t, err) { return } diff --git a/cmd/rp/cmd/run.go b/cmd/rp/cmd/run.go index ea1e940..34db06f 100644 --- a/cmd/rp/cmd/run.go +++ b/cmd/rp/cmd/run.go @@ -59,7 +59,7 @@ func run(cmd *cobra.Command, _ []string) error { }) } - releaserPleaser := rp.New(forge, logger, flagBranch) + releaserPleaser := rp.New(forge, logger, flagBranch, rp.NewConventionalCommitsParser(), rp.SemVerNextVersion) return releaserPleaser.Run(ctx) } diff --git a/commits.go b/commits.go index 28f9f4a..f0c64e9 100644 --- a/commits.go +++ b/commits.go @@ -7,6 +7,19 @@ import ( "github.com/leodido/go-conventionalcommits/parser" ) +type Commit struct { + Hash string + Message string + + PullRequest *PullRequest +} + +type PullRequest struct { + ID int + Title string + Description string +} + type AnalyzedCommit struct { Commit Type string @@ -34,7 +47,7 @@ func NewConventionalCommitsParser() *ConventionalCommitsParser { } } -func (c *ConventionalCommitsParser) AnalyzeCommits(commits []Commit) ([]AnalyzedCommit, error) { +func (c *ConventionalCommitsParser) Analyze(commits []Commit) ([]AnalyzedCommit, error) { analyzedCommits := make([]AnalyzedCommit, 0, len(commits)) for _, commit := range commits { diff --git a/commits_test.go b/commits_test.go index 0725b32..e58a718 100644 --- a/commits_test.go +++ b/commits_test.go @@ -111,7 +111,7 @@ func TestAnalyzeCommits(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - analyzedCommits, err := NewConventionalCommitsParser().AnalyzeCommits(tt.commits) + analyzedCommits, err := NewConventionalCommitsParser().Analyze(tt.commits) if !tt.wantErr(t, err) { return } diff --git a/forge.go b/forge.go index 41e7956..c244a0c 100644 --- a/forge.go +++ b/forge.go @@ -25,12 +25,6 @@ const ( GitHubLabelColor = "dedede" ) -type Changeset struct { - URL string - Identifier string - ChangelogEntries []AnalyzedCommit -} - type Forge interface { RepoURL() string CloneURL() string @@ -46,9 +40,6 @@ type Forge interface { // function should return all commits. CommitsSince(context.Context, *Tag) ([]Commit, error) - // Changesets looks up the Pull/Merge Requests for each commit, returning its parsed metadata. - Changesets(context.Context, []Commit) ([]Changeset, error) - // EnsureLabelsExist verifies that all desired labels are available on the repository. If labels are missing, they // are created them. EnsureLabelsExist(context.Context, []Label) error @@ -184,10 +175,16 @@ func (g *GitHub) CommitsSince(ctx context.Context, tag *Tag) ([]Commit, error) { var commits = make([]Commit, 0, len(repositoryCommits)) for _, ghCommit := range repositoryCommits { - commits = append(commits, Commit{ + commit := Commit{ Hash: ghCommit.GetSHA(), Message: ghCommit.GetCommit().GetMessage(), - }) + } + commit.PullRequest, err = g.prForCommit(ctx, commit) + if err != nil { + return nil, fmt.Errorf("failed to check for commit pull request: %w", err) + } + + commits = append(commits, commit) } return commits, nil @@ -272,73 +269,49 @@ func (g *GitHub) commitsSinceInit(ctx context.Context) ([]*github.RepositoryComm return repositoryCommits, nil } -func (g *GitHub) Changesets(ctx context.Context, commits []Commit) ([]Changeset, error) { +func (g *GitHub) prForCommit(ctx context.Context, commit Commit) (*PullRequest, error) { // We naively look up the associated PR for each commit through the "List pull requests associated with a commit" // endpoint. This requires len(commits) requests. // Using the "List pull requests" endpoint might be faster, as it allows us to fetch 100 arbitrary PRs per request, // but worst case we need to look up all PRs made in the repository ever. - changesets := make([]Changeset, 0, len(commits)) + log := g.log.With("commit.hash", commit.Hash) + page := 1 + var associatedPRs []*github.PullRequest - for _, commit := range commits { - log := g.log.With("commit.hash", commit.Hash) - page := 1 - var associatedPRs []*github.PullRequest - - for { - log.Debug("fetching pull requests associated with commit", "page", page) - prs, resp, err := g.client.PullRequests.ListPullRequestsWithCommit( - ctx, g.options.Owner, g.options.Repo, - commit.Hash, &github.ListOptions{ - Page: page, - PerPage: GitHubPerPageMax, - }) - if err != nil { - return nil, err - } - - associatedPRs = append(associatedPRs, prs...) - - if page == resp.LastPage || resp.LastPage == 0 { - break - } - page = resp.NextPage - } - - var pullrequest *github.PullRequest - for _, pr := range associatedPRs { - // We only look for the PR that has this commit set as the "merge commit" => The result of squashing this branch onto main - if pr.GetMergeCommitSHA() == commit.Hash { - pullrequest = pr - break - } - } - if pullrequest == nil { - log.Warn("did not find associated pull request, not considering it for changesets") - // No pull request was found for this commit, nothing to do here - // TODO: We could also return the minimal changeset for this commit, so at least it would come up in the changelog. - continue - } - - log = log.With("pullrequest.id", pullrequest.GetID()) - - // TODO: Parse PR description for overrides - changelogEntries, err := NewConventionalCommitsParser().AnalyzeCommits([]Commit{commit}) - if err != nil { - log.Warn("unable to parse changelog entries", "error", err) - continue - } - - if len(changelogEntries) > 0 { - changesets = append(changesets, Changeset{ - URL: pullrequest.GetHTMLURL(), - Identifier: fmt.Sprintf("#%d", pullrequest.GetNumber()), - ChangelogEntries: changelogEntries, + for { + log.Debug("fetching pull requests associated with commit", "page", page) + prs, resp, err := g.client.PullRequests.ListPullRequestsWithCommit( + ctx, g.options.Owner, g.options.Repo, + commit.Hash, &github.ListOptions{ + Page: page, + PerPage: GitHubPerPageMax, }) + if err != nil { + return nil, err } + + associatedPRs = append(associatedPRs, prs...) + + if page == resp.LastPage || resp.LastPage == 0 { + break + } + page = resp.NextPage } - return changesets, nil + var pullrequest *github.PullRequest + for _, pr := range associatedPRs { + // We only look for the PR that has this commit set as the "merge commit" => The result of squashing this branch onto main + if pr.GetMergeCommitSHA() == commit.Hash { + pullrequest = pr + break + } + } + if pullrequest == nil { + return nil, nil + } + + return gitHubPRToPullRequest(pullrequest), nil } func (g *GitHub) EnsureLabelsExist(ctx context.Context, labels []Label) error { @@ -578,6 +551,14 @@ func (g *GitHub) CreateRelease(ctx context.Context, commit Commit, title, change return nil } +func gitHubPRToPullRequest(pr *github.PullRequest) *PullRequest { + return &PullRequest{ + ID: pr.GetNumber(), + Title: pr.GetTitle(), + Description: pr.GetBody(), + } +} + func gitHubPRToReleasePullRequest(pr *github.PullRequest) *ReleasePullRequest { labels := make([]Label, 0, len(pr.Labels)) for _, label := range pr.Labels { diff --git a/git.go b/git.go index 7df742c..2f0c7f2 100644 --- a/git.go +++ b/git.go @@ -17,11 +17,6 @@ const ( GitRemoteName = "origin" ) -type Commit struct { - Hash string - Message string -} - type Tag struct { Hash string Name string diff --git a/releasepr.go b/releasepr.go index 1e4d3a8..1d41325 100644 --- a/releasepr.go +++ b/releasepr.go @@ -31,6 +31,9 @@ func init() { } } +// ReleasePullRequest +// +// TODO: Reuse [PullRequest] type ReleasePullRequest struct { ID int Title string @@ -58,7 +61,7 @@ func NewReleasePullRequest(head, branch, version, changelogEntry string) (*Relea type ReleaseOverrides struct { Prefix string Suffix string - // TODO: Doing the changelog for normal releases after previews requires to know about this while fetching the changesets + // TODO: Doing the changelog for normal releases after previews requires to know about this while fetching the commits NextVersionType NextVersionType } diff --git a/releaserpleaser.go b/releaserpleaser.go index 554e0fd..7c3ab16 100644 --- a/releaserpleaser.go +++ b/releaserpleaser.go @@ -18,13 +18,17 @@ type ReleaserPleaser struct { forge Forge logger *slog.Logger targetBranch string + commitParser CommitParser + nextVersion VersioningStrategy } -func New(forge Forge, logger *slog.Logger, targetBranch string) *ReleaserPleaser { +func New(forge Forge, logger *slog.Logger, targetBranch string, commitParser CommitParser, versioningStrategy VersioningStrategy) *ReleaserPleaser { return &ReleaserPleaser{ forge: forge, logger: logger, targetBranch: targetBranch, + commitParser: commitParser, + nextVersion: versioningStrategy, } } @@ -154,12 +158,13 @@ func (rp *ReleaserPleaser) runReconcileReleasePR(ctx context.Context) error { logger.InfoContext(ctx, "Found releasable commits", "length", len(releasableCommits)) - changesets, err := rp.forge.Changesets(ctx, releasableCommits) + // TODO: Handle commit overrides + analyzedCommits, err := rp.commitParser.Analyze(releasableCommits) if err != nil { return err } - logger.InfoContext(ctx, "Found changesets", "length", len(changesets)) + logger.InfoContext(ctx, "Analyzed commits", "length", len(analyzedCommits)) rpBranch := fmt.Sprintf(PullRequestBranchFormat, rp.targetBranch) rpBranchRef := plumbing.NewBranchReferenceName(rpBranch) @@ -177,15 +182,15 @@ func (rp *ReleaserPleaser) runReconcileReleasePR(ctx context.Context) error { logger.InfoContext(ctx, "found existing release pull request", "pr.id", pr.ID, "pr.title", pr.Title) } - if len(changesets) == 0 { + if len(analyzedCommits) == 0 { if pr != nil { - logger.InfoContext(ctx, "closing existing pull requests, no changesets available", "pr.id", pr.ID, "pr.title", pr.Title) + logger.InfoContext(ctx, "closing existing pull requests, no commits available", "pr.id", pr.ID, "pr.title", pr.Title) err = rp.forge.ClosePullRequest(ctx, pr) if err != nil { return err } } else { - logger.InfoContext(ctx, "No changesets available for release") + logger.InfoContext(ctx, "No commits available for release") } return nil @@ -199,8 +204,9 @@ func (rp *ReleaserPleaser) runReconcileReleasePR(ctx context.Context) error { } } - versionBump := VersionBumpFromChangesets(changesets) - nextVersion, err := SemVerNextVersion(releases, versionBump, releaseOverrides.NextVersionType) + versionBump := VersionBumpFromCommits(analyzedCommits) + // TODO: Set version in release pr + nextVersion, err := rp.nextVersion(releases, versionBump, releaseOverrides.NextVersionType) if err != nil { return err } @@ -235,7 +241,7 @@ func (rp *ReleaserPleaser) runReconcileReleasePR(ctx context.Context) error { return fmt.Errorf("failed to update files with new version: %w", err) } - changelogEntry, err := NewChangelogEntry(changesets, nextVersion, rp.forge.ReleaseURL(nextVersion), releaseOverrides.Prefix, releaseOverrides.Suffix) + changelogEntry, err := NewChangelogEntry(analyzedCommits, nextVersion, rp.forge.ReleaseURL(nextVersion), releaseOverrides.Prefix, releaseOverrides.Suffix) if err != nil { return fmt.Errorf("failed to build changelog entry: %w", err) } diff --git a/versioning.go b/versioning.go index 77bb96f..176a28d 100644 --- a/versioning.go +++ b/versioning.go @@ -69,24 +69,22 @@ func SemVerNextVersion(r Releases, versionBump conventionalcommits.VersionBump, return "v" + next.String(), nil } -func VersionBumpFromChangesets(changesets []Changeset) conventionalcommits.VersionBump { +func VersionBumpFromCommits(commits []AnalyzedCommit) conventionalcommits.VersionBump { bump := conventionalcommits.UnknownVersion - for _, changeset := range changesets { - for _, entry := range changeset.ChangelogEntries { - entryBump := conventionalcommits.UnknownVersion - switch { - case entry.BreakingChange: - entryBump = conventionalcommits.MajorVersion - case entry.Type == "feat": - entryBump = conventionalcommits.MinorVersion - case entry.Type == "fix": - entryBump = conventionalcommits.PatchVersion - } + for _, commit := range commits { + entryBump := conventionalcommits.UnknownVersion + switch { + case commit.BreakingChange: + entryBump = conventionalcommits.MajorVersion + case commit.Type == "feat": + entryBump = conventionalcommits.MinorVersion + case commit.Type == "fix": + entryBump = conventionalcommits.PatchVersion + } - if entryBump > bump { - bump = entryBump - } + if entryBump > bump { + bump = entryBump } } diff --git a/versioning_test.go b/versioning_test.go index d1846d8..b6a0995 100644 --- a/versioning_test.go +++ b/versioning_test.go @@ -333,86 +333,56 @@ func TestReleases_NextVersion(t *testing.T) { } } -func TestVersionBumpFromChangesets(t *testing.T) { +func TestVersionBumpFromCommits(t *testing.T) { tests := []struct { - name string - changesets []Changeset - want conventionalcommits.VersionBump + name string + analyzedCommits []AnalyzedCommit + want conventionalcommits.VersionBump }{ { - name: "no entries (unknown)", - changesets: []Changeset{}, - want: conventionalcommits.UnknownVersion, + name: "no entries (unknown)", + analyzedCommits: []AnalyzedCommit{}, + want: conventionalcommits.UnknownVersion, }, { - name: "non-release type (unknown)", - changesets: []Changeset{{ChangelogEntries: []AnalyzedCommit{{Type: "docs"}}}}, - want: conventionalcommits.UnknownVersion, + name: "non-release type (unknown)", + analyzedCommits: []AnalyzedCommit{{Type: "docs"}}, + want: conventionalcommits.UnknownVersion, }, { - name: "single breaking (major)", - changesets: []Changeset{{ChangelogEntries: []AnalyzedCommit{{BreakingChange: true}}}}, - want: conventionalcommits.MajorVersion, + name: "single breaking (major)", + analyzedCommits: []AnalyzedCommit{{BreakingChange: true}}, + want: conventionalcommits.MajorVersion, }, { - name: "single feat (minor)", - changesets: []Changeset{{ChangelogEntries: []AnalyzedCommit{{Type: "feat"}}}}, - want: conventionalcommits.MinorVersion, + name: "single feat (minor)", + analyzedCommits: []AnalyzedCommit{{Type: "feat"}}, + want: conventionalcommits.MinorVersion, }, { - name: "single fix (patch)", - changesets: []Changeset{{ChangelogEntries: []AnalyzedCommit{{Type: "fix"}}}}, - want: conventionalcommits.PatchVersion, + name: "single fix (patch)", + analyzedCommits: []AnalyzedCommit{{Type: "fix"}}, + want: conventionalcommits.PatchVersion, }, { - name: "multiple changesets (major)", - changesets: []Changeset{ - {ChangelogEntries: []AnalyzedCommit{{Type: "fix"}}}, - {ChangelogEntries: []AnalyzedCommit{{BreakingChange: true}}}, - }, - want: conventionalcommits.MajorVersion, + name: "multiple entries (major)", + analyzedCommits: []AnalyzedCommit{{Type: "fix"}, {BreakingChange: true}}, + want: conventionalcommits.MajorVersion, }, { - name: "multiple changesets (minor)", - changesets: []Changeset{ - {ChangelogEntries: []AnalyzedCommit{{Type: "fix"}}}, - {ChangelogEntries: []AnalyzedCommit{{Type: "feat"}}}, - }, - want: conventionalcommits.MinorVersion, + name: "multiple entries (minor)", + analyzedCommits: []AnalyzedCommit{{Type: "fix"}, {Type: "feat"}}, + want: conventionalcommits.MinorVersion, }, { - name: "multiple changesets (patch)", - changesets: []Changeset{ - {ChangelogEntries: []AnalyzedCommit{{Type: "docs"}}}, - {ChangelogEntries: []AnalyzedCommit{{Type: "fix"}}}, - }, - want: conventionalcommits.PatchVersion, - }, - { - name: "multiple entries in one changeset (major)", - changesets: []Changeset{ - {ChangelogEntries: []AnalyzedCommit{{Type: "fix"}, {BreakingChange: true}}}, - }, - want: conventionalcommits.MajorVersion, - }, - { - name: "multiple entries in one changeset (minor)", - changesets: []Changeset{ - {ChangelogEntries: []AnalyzedCommit{{Type: "fix"}, {Type: "feat"}}}, - }, - want: conventionalcommits.MinorVersion, - }, - { - name: "multiple entries in one changeset (patch)", - changesets: []Changeset{ - {ChangelogEntries: []AnalyzedCommit{{Type: "docs"}, {Type: "fix"}}}, - }, - want: conventionalcommits.PatchVersion, + name: "multiple entries (patch)", + analyzedCommits: []AnalyzedCommit{{Type: "docs"}, {Type: "fix"}}, + want: conventionalcommits.PatchVersion, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equalf(t, tt.want, VersionBumpFromChangesets(tt.changesets), "VersionBumpFromChangesets(%v)", tt.changesets) + assert.Equalf(t, tt.want, VersionBumpFromCommits(tt.analyzedCommits), "VersionBumpFromCommits(%v)", tt.analyzedCommits) }) } }