From 464064460e26b25836a7da914e03b1aec478c287 Mon Sep 17 00:00:00 2001 From: Keith Zantow Date: Mon, 23 Jan 2023 17:11:18 -0500 Subject: [PATCH 1/8] feat: add authors for issue entries, display unlabeled PRs Signed-off-by: Keith Zantow --- chronicle/release/change/change.go | 4 +- .../release/format/markdown/presenter_test.go | 4 + .../releasers/github/gh_pull_request.go | 23 +++ .../release/releasers/github/summarizer.go | 149 ++++++++++++------ internal/config/github.go | 15 +- internal/git/remote.go | 2 +- 6 files changed, 144 insertions(+), 53 deletions(-) diff --git a/chronicle/release/change/change.go b/chronicle/release/change/change.go index 48ad9d8..5a38032 100644 --- a/chronicle/release/change/change.go +++ b/chronicle/release/change/change.go @@ -4,6 +4,8 @@ import ( "time" ) +const UnlabeledPRs = "unlabeled-prs" + type Changes []Change // Change represents the smallest unit within a release that can be summarized. @@ -25,7 +27,7 @@ type Reference struct { // ByChangeType returns the set of changes that match one of the given change types. func (s Changes) ByChangeType(types ...Type) (result Changes) { for _, summary := range s { - if ContainsAny(types, summary.ChangeTypes) { + if ContainsAny(types, summary.ChangeTypes) || (len(summary.ChangeTypes) == 0 && types[0].Name == UnlabeledPRs) { result = append(result, summary) } } diff --git a/chronicle/release/format/markdown/presenter_test.go b/chronicle/release/format/markdown/presenter_test.go index d4c5ad6..e9de8ee 100644 --- a/chronicle/release/format/markdown/presenter_test.go +++ b/chronicle/release/format/markdown/presenter_test.go @@ -47,6 +47,10 @@ func TestMarkdownPresenter_Present(t *testing.T) { ChangeType: change.NewType("removed", change.SemVerMajor), Title: "Removed Features", }, + { + ChangeType: change.NewType("unlabeled", change.SemVerMajor), + Title: "Unlabeled PRs", + }, }, Release: release.Release{ Version: "v0.19.1", diff --git a/chronicle/release/releasers/github/gh_pull_request.go b/chronicle/release/releasers/github/gh_pull_request.go index d48a4c0..5c821db 100644 --- a/chronicle/release/releasers/github/gh_pull_request.go +++ b/chronicle/release/releasers/github/gh_pull_request.go @@ -161,6 +161,29 @@ func prsWithLabel(labels ...string) prFilter { } } +func prsUnlabeled() prFilter { + return func(pr ghPullRequest) bool { + keep := len(pr.Labels) == 0 + if !keep { + log.Tracef("PR #%d filtered out: unlabeled (merged %s)", pr.Number, internal.FormatDateTime(pr.MergedAt)) + } + return keep + } +} + +func prsUnlinked() prFilter { + return func(pr ghPullRequest) bool { + return len(pr.LinkedIssues) == 0 + } +} + +func prsWithChangeTypes(config Config) prFilter { + return func(pr ghPullRequest) bool { + changeTypes := config.ChangeTypesByLabel.ChangeTypes(pr.Labels...) + return len(changeTypes) > 0 + } +} + func prsWithoutLabel(labels ...string) prFilter { return func(pr ghPullRequest) bool { for _, targetLabel := range labels { diff --git a/chronicle/release/releasers/github/summarizer.go b/chronicle/release/releasers/github/summarizer.go index 56584cd..0e7b43f 100644 --- a/chronicle/release/releasers/github/summarizer.go +++ b/chronicle/release/releasers/github/summarizer.go @@ -22,10 +22,12 @@ var _ release.Summarizer = (*Summarizer)(nil) type Config struct { Host string IncludeIssues bool + IncludeIssuePRAuthors bool IncludePRs bool + IncludeUnlabeledPRs bool + IssuesRequireLinkedPR bool ExcludeLabels []string ChangeTypesByLabel change.TypeSet - IssuesRequireLinkedPR bool ConsiderPRMergeCommits bool } @@ -145,35 +147,39 @@ func (s *Summarizer) Changes(sinceRef, untilRef string) ([]change.Change, error) logCommits(includeCommits) } - if s.config.IncludePRs || (s.config.IssuesRequireLinkedPR && s.config.IncludeIssues) { - allMergedPRs, err := fetchMergedPRs(s.userName, s.repoName) - if err != nil { - return nil, err - } + allMergedPRs, err := fetchMergedPRs(s.userName, s.repoName) + if err != nil { + return nil, err + } - log.Debugf("total merged PRs discovered: %d", len(allMergedPRs)) + log.Debugf("total merged PRs discovered: %d", len(allMergedPRs)) + if s.config.IncludePRs || (s.config.IssuesRequireLinkedPR && s.config.IncludeIssues) { if s.config.IncludePRs { - changes = append(changes, changesFromPRs(s.config, allMergedPRs, sinceTag, untilTag, includeCommits)...) + changes = append(changes, changesFromStandardPRFilters(s.config, allMergedPRs, sinceTag, untilTag, includeCommits)...) } if s.config.IssuesRequireLinkedPR && s.config.IncludeIssues { // extract closed linked issues with closed PRs from the PR list. Why do this here? // githubs ontology has PRs as the source of truth for issue linking. Linked PR information // is not available on the issue itself. extractedIssues := issuesExtractedFromPRs(s.config, allMergedPRs, sinceTag, untilTag, includeCommits) - changes = append(changes, createChangesFromIssues(s.config, extractedIssues)...) + changes = append(changes, createChangesFromIssues(s.config, allMergedPRs, extractedIssues, filterIssuesByChangeTypes)...) } } - if s.config.IncludeIssues && !s.config.IssuesRequireLinkedPR { - allClosedIssues, err := fetchClosedIssues(s.userName, s.repoName) - if err != nil { - return nil, err - } + allClosedIssues, err := fetchClosedIssues(s.userName, s.repoName) + if err != nil { + return nil, err + } + + log.Debugf("total closed issues discovered: %d", len(allClosedIssues)) - log.Debugf("total closed issues discovered: %d", len(allClosedIssues)) + if s.config.IncludeIssues && !s.config.IssuesRequireLinkedPR { + changes = append(changes, changesFromIssues(s.config, allMergedPRs, allClosedIssues, sinceTag, untilTag)...) + } - changes = append(changes, changesFromIssues(s.config, allClosedIssues, sinceTag, untilTag)...) + if s.config.IncludeUnlabeledPRs { + changes = append(changes, changesFromUnlabeledPRs(s.config, allMergedPRs, sinceTag, untilTag)...) } return changes, nil @@ -234,34 +240,38 @@ func uniqueIssuesFromPRs(prs []ghPullRequest) []ghIssue { return issues } -func changesFromPRs(config Config, allMergedPRs []ghPullRequest, sinceTag, untilTag *git.Tag, includeCommits []string) []change.Change { +func changesFromStandardPRFilters(config Config, allMergedPRs []ghPullRequest, sinceTag, untilTag *git.Tag, includeCommits []string) []change.Change { includedPRs := applyStandardPRFilters(allMergedPRs, config, sinceTag, untilTag, includeCommits) + includedPRs, _ = filterPRs(includedPRs, prsWithChangeTypes(config)) + log.Debugf("PRs contributing to changelog: %d", len(includedPRs)) logPRs(includedPRs) + return changesFromPRs(config, includedPRs) +} + +func changesFromPRs(config Config, prs []ghPullRequest) []change.Change { var summaries []change.Change - for _, pr := range includedPRs { + for _, pr := range prs { changeTypes := config.ChangeTypesByLabel.ChangeTypes(pr.Labels...) - if len(changeTypes) > 0 { - summaries = append(summaries, change.Change{ - Text: pr.Title, - ChangeTypes: changeTypes, - Timestamp: pr.MergedAt, - References: []change.Reference{ - { - Text: fmt.Sprintf("PR #%d", pr.Number), - URL: pr.URL, - }, - { - Text: pr.Author, - URL: fmt.Sprintf("https://%s/%s", config.Host, pr.Author), - }, + summaries = append(summaries, change.Change{ + Text: pr.Title, + ChangeTypes: changeTypes, + Timestamp: pr.MergedAt, + References: []change.Reference{ + { + Text: fmt.Sprintf("PR #%d", pr.Number), + URL: pr.URL, }, - EntryType: "githubPR", - Entry: pr, - }) - } + { + Text: pr.Author, + URL: fmt.Sprintf("https://%s/%s", config.Host, pr.Author), + }, + }, + EntryType: "githubPR", + Entry: pr, + }) } return summaries } @@ -276,13 +286,18 @@ func logPRs(prs []ghPullRequest) { } } -func changesFromIssues(config Config, allClosedIssues []ghIssue, sinceTag, untilTag *git.Tag) []change.Change { +func changesFromIssues(config Config, allMergedPRs []ghPullRequest, allClosedIssues []ghIssue, sinceTag, untilTag *git.Tag) []change.Change { filteredIssues := filterIssues(allClosedIssues, standardIssueFilters(config, sinceTag, untilTag)...) log.Debugf("issues contributing to changelog: %d", len(filteredIssues)) logIssues(filteredIssues) - return createChangesFromIssues(config, filteredIssues) + return createChangesFromIssues(config, allMergedPRs, filteredIssues, filterIssuesByChangeTypes) +} + +func filterIssuesByChangeTypes(config Config, issue ghIssue) bool { + changeTypes := config.ChangeTypesByLabel.ChangeTypes(issue.Labels...) + return len(changeTypes) > 0 } func logIssues(issues []ghIssue) { @@ -295,23 +310,57 @@ func logIssues(issues []ghIssue) { } } -func createChangesFromIssues(config Config, issues []ghIssue) (changes []change.Change) { +func changesFromUnlabeledPRs(config Config, allMergedPRs []ghPullRequest, sinceTag, untilTag *git.Tag) []change.Change { + // this represents the traits we wish to filter down to (not out). + filters := []prFilter{ + prsAfter(sinceTag.Timestamp), + prsUnlabeled(), + prsUnlinked(), + } + + if untilTag != nil { + filters = append(filters, prsAtOrBefore(untilTag.Timestamp)) + } + + filteredIssues, _ := filterPRs(allMergedPRs, filters...) + + log.Debugf("prs contributing to changelog: %d", len(filteredIssues)) + + return changesFromPRs(config, filteredIssues) +} + +func createChangesFromIssues(config Config, allMergedPRs []ghPullRequest, issues []ghIssue, filter func(Config, ghIssue) bool) (changes []change.Change) { for _, issue := range issues { - changeTypes := config.ChangeTypesByLabel.ChangeTypes(issue.Labels...) - if len(changeTypes) > 0 { + if filter(config, issue) { + changeTypes := config.ChangeTypesByLabel.ChangeTypes(issue.Labels...) + + references := []change.Reference{ + { + Text: fmt.Sprintf("Issue #%d", issue.Number), + URL: issue.URL, + }, + } + + if config.IncludeIssuePRAuthors { + for _, pr := range allMergedPRs { + for _, linkedIssue := range pr.LinkedIssues { + if linkedIssue.URL == issue.URL { + references = append(references, change.Reference{ + Text: pr.Author, + URL: fmt.Sprintf("https://%s/%s", config.Host, pr.Author), + }) + } + } + } + } + changes = append(changes, change.Change{ Text: issue.Title, ChangeTypes: changeTypes, Timestamp: issue.ClosedAt, - References: []change.Reference{ - { - Text: fmt.Sprintf("Issue #%d", issue.Number), - URL: issue.URL, - }, - // TODO: add assignee(s) name + url - }, - EntryType: "githubIssue", - Entry: issue, + References: references, + EntryType: "githubIssue", + Entry: issue, }) } } diff --git a/internal/config/github.go b/internal/config/github.go index 0683575..d792413 100644 --- a/internal/config/github.go +++ b/internal/config/github.go @@ -12,8 +12,10 @@ import ( type githubSummarizer struct { Host string `yaml:"host" json:"host" mapstructure:"host"` ExcludeLabels []string `yaml:"exclude-labels" json:"exclude-labels" mapstructure:"exclude-labels"` - IncludePRs bool `yaml:"include-prs" json:"include-prs" mapstructure:"include-prs"` + IncludeIssuePRAuthors bool `yaml:"include-issue-pr-authors" json:"include-issue-pr-authors" mapstructure:"include-issue-pr-authors"` IncludeIssues bool `yaml:"include-issues" json:"include-issues" mapstructure:"include-issues"` + IncludePRs bool `yaml:"include-prs" json:"include-prs" mapstructure:"include-prs"` + IncludeUnlabeledPRs bool `yaml:"include-unlabeled-prs" json:"include-unlabeled-prs" mapstructure:"include-unlabeled-prs"` IssuesRequireLinkedPR bool `yaml:"issues-require-linked-prs" json:"issues-require-linked-prs" mapstructure:"issues-require-linked-prs"` ConsiderPRMergeCommits bool `yaml:"consider-pr-merge-commits" json:"consider-pr-merge-commits" mapstructure:"consider-pr-merge-commits"` Changes []githubChange `yaml:"changes" json:"changes" mapstructure:"changes"` @@ -42,8 +44,10 @@ func (cfg githubSummarizer) ToGithubConfig() (github.Config, error) { Host: cfg.Host, IncludeIssues: cfg.IncludeIssues, IncludePRs: cfg.IncludePRs, + IncludeUnlabeledPRs: cfg.IncludeUnlabeledPRs, ExcludeLabels: cfg.ExcludeLabels, IssuesRequireLinkedPR: cfg.IssuesRequireLinkedPR, + IncludeIssuePRAuthors: cfg.IncludeIssuePRAuthors, ConsiderPRMergeCommits: cfg.ConsiderPRMergeCommits, ChangeTypesByLabel: typeSet, }, nil @@ -52,9 +56,12 @@ func (cfg githubSummarizer) ToGithubConfig() (github.Config, error) { func (cfg githubSummarizer) loadDefaultValues(v *viper.Viper) { v.SetDefault("github.host", "github.com") v.SetDefault("github.issues-require-linked-prs", false) + v.SetDefault("github.include-issue-pr-authors", true) v.SetDefault("github.consider-pr-merge-commits", true) v.SetDefault("github.include-prs", true) v.SetDefault("github.include-issues", true) + v.SetDefault("github.include-prs", true) + v.SetDefault("github.include-unlabeled-prs", true) v.SetDefault("github.exclude-labels", []string{"duplicate", "question", "invalid", "wontfix", "wont-fix", "release-ignore", "changelog-ignore", "ignore"}) v.SetDefault("github.changes", []githubChange{ { @@ -93,5 +100,11 @@ func (cfg githubSummarizer) loadDefaultValues(v *viper.Viper) { Labels: []string{"deprecated"}, SemVerKind: change.SemVerMinor.String(), }, + { + Type: change.UnlabeledPRs, + Title: "Unlabeled PRs", + Labels: []string{""}, + SemVerKind: change.SemVerMinor.String(), + }, }) } diff --git a/internal/git/remote.go b/internal/git/remote.go index a83852c..2b834b7 100644 --- a/internal/git/remote.go +++ b/internal/git/remote.go @@ -10,7 +10,7 @@ import ( "github.com/anchore/chronicle/internal" ) -var remotePattern = regexp.MustCompile(`(?m)\[remote "origin"](\n.*)*url\s*=\s*(?P.*)$`) +var remotePattern = regexp.MustCompile(`\[remote "origin"]\s*\n\s*url\s*=\s*(?P[^\s]+)\s+`) // TODO: can't use r.Config for same validation reasons func RemoteURL(p string) (string, error) { From ff19f0fdca88b2ce7b4b80482614e033a9a5214d Mon Sep 17 00:00:00 2001 From: Keith Zantow Date: Mon, 23 Jan 2023 20:11:21 -0500 Subject: [PATCH 2/8] feat: refactor out unlabeled prs change Signed-off-by: Keith Zantow --- chronicle/release/change/change.go | 4 +-- .../release/format/markdown/presenter_test.go | 4 --- .../releasers/github/gh_pull_request.go | 16 ------------ .../release/releasers/github/summarizer.go | 26 +------------------ internal/config/github.go | 14 ++-------- 5 files changed, 4 insertions(+), 60 deletions(-) diff --git a/chronicle/release/change/change.go b/chronicle/release/change/change.go index 5a38032..48ad9d8 100644 --- a/chronicle/release/change/change.go +++ b/chronicle/release/change/change.go @@ -4,8 +4,6 @@ import ( "time" ) -const UnlabeledPRs = "unlabeled-prs" - type Changes []Change // Change represents the smallest unit within a release that can be summarized. @@ -27,7 +25,7 @@ type Reference struct { // ByChangeType returns the set of changes that match one of the given change types. func (s Changes) ByChangeType(types ...Type) (result Changes) { for _, summary := range s { - if ContainsAny(types, summary.ChangeTypes) || (len(summary.ChangeTypes) == 0 && types[0].Name == UnlabeledPRs) { + if ContainsAny(types, summary.ChangeTypes) { result = append(result, summary) } } diff --git a/chronicle/release/format/markdown/presenter_test.go b/chronicle/release/format/markdown/presenter_test.go index e9de8ee..d4c5ad6 100644 --- a/chronicle/release/format/markdown/presenter_test.go +++ b/chronicle/release/format/markdown/presenter_test.go @@ -47,10 +47,6 @@ func TestMarkdownPresenter_Present(t *testing.T) { ChangeType: change.NewType("removed", change.SemVerMajor), Title: "Removed Features", }, - { - ChangeType: change.NewType("unlabeled", change.SemVerMajor), - Title: "Unlabeled PRs", - }, }, Release: release.Release{ Version: "v0.19.1", diff --git a/chronicle/release/releasers/github/gh_pull_request.go b/chronicle/release/releasers/github/gh_pull_request.go index 5c821db..e476ce9 100644 --- a/chronicle/release/releasers/github/gh_pull_request.go +++ b/chronicle/release/releasers/github/gh_pull_request.go @@ -161,22 +161,6 @@ func prsWithLabel(labels ...string) prFilter { } } -func prsUnlabeled() prFilter { - return func(pr ghPullRequest) bool { - keep := len(pr.Labels) == 0 - if !keep { - log.Tracef("PR #%d filtered out: unlabeled (merged %s)", pr.Number, internal.FormatDateTime(pr.MergedAt)) - } - return keep - } -} - -func prsUnlinked() prFilter { - return func(pr ghPullRequest) bool { - return len(pr.LinkedIssues) == 0 - } -} - func prsWithChangeTypes(config Config) prFilter { return func(pr ghPullRequest) bool { changeTypes := config.ChangeTypesByLabel.ChangeTypes(pr.Labels...) diff --git a/chronicle/release/releasers/github/summarizer.go b/chronicle/release/releasers/github/summarizer.go index 0e7b43f..13873f6 100644 --- a/chronicle/release/releasers/github/summarizer.go +++ b/chronicle/release/releasers/github/summarizer.go @@ -21,10 +21,9 @@ var _ release.Summarizer = (*Summarizer)(nil) type Config struct { Host string - IncludeIssues bool IncludeIssuePRAuthors bool + IncludeIssues bool IncludePRs bool - IncludeUnlabeledPRs bool IssuesRequireLinkedPR bool ExcludeLabels []string ChangeTypesByLabel change.TypeSet @@ -178,10 +177,6 @@ func (s *Summarizer) Changes(sinceRef, untilRef string) ([]change.Change, error) changes = append(changes, changesFromIssues(s.config, allMergedPRs, allClosedIssues, sinceTag, untilTag)...) } - if s.config.IncludeUnlabeledPRs { - changes = append(changes, changesFromUnlabeledPRs(s.config, allMergedPRs, sinceTag, untilTag)...) - } - return changes, nil } @@ -310,25 +305,6 @@ func logIssues(issues []ghIssue) { } } -func changesFromUnlabeledPRs(config Config, allMergedPRs []ghPullRequest, sinceTag, untilTag *git.Tag) []change.Change { - // this represents the traits we wish to filter down to (not out). - filters := []prFilter{ - prsAfter(sinceTag.Timestamp), - prsUnlabeled(), - prsUnlinked(), - } - - if untilTag != nil { - filters = append(filters, prsAtOrBefore(untilTag.Timestamp)) - } - - filteredIssues, _ := filterPRs(allMergedPRs, filters...) - - log.Debugf("prs contributing to changelog: %d", len(filteredIssues)) - - return changesFromPRs(config, filteredIssues) -} - func createChangesFromIssues(config Config, allMergedPRs []ghPullRequest, issues []ghIssue, filter func(Config, ghIssue) bool) (changes []change.Change) { for _, issue := range issues { if filter(config, issue) { diff --git a/internal/config/github.go b/internal/config/github.go index d792413..0584749 100644 --- a/internal/config/github.go +++ b/internal/config/github.go @@ -15,7 +15,6 @@ type githubSummarizer struct { IncludeIssuePRAuthors bool `yaml:"include-issue-pr-authors" json:"include-issue-pr-authors" mapstructure:"include-issue-pr-authors"` IncludeIssues bool `yaml:"include-issues" json:"include-issues" mapstructure:"include-issues"` IncludePRs bool `yaml:"include-prs" json:"include-prs" mapstructure:"include-prs"` - IncludeUnlabeledPRs bool `yaml:"include-unlabeled-prs" json:"include-unlabeled-prs" mapstructure:"include-unlabeled-prs"` IssuesRequireLinkedPR bool `yaml:"issues-require-linked-prs" json:"issues-require-linked-prs" mapstructure:"issues-require-linked-prs"` ConsiderPRMergeCommits bool `yaml:"consider-pr-merge-commits" json:"consider-pr-merge-commits" mapstructure:"consider-pr-merge-commits"` Changes []githubChange `yaml:"changes" json:"changes" mapstructure:"changes"` @@ -44,7 +43,6 @@ func (cfg githubSummarizer) ToGithubConfig() (github.Config, error) { Host: cfg.Host, IncludeIssues: cfg.IncludeIssues, IncludePRs: cfg.IncludePRs, - IncludeUnlabeledPRs: cfg.IncludeUnlabeledPRs, ExcludeLabels: cfg.ExcludeLabels, IssuesRequireLinkedPR: cfg.IssuesRequireLinkedPR, IncludeIssuePRAuthors: cfg.IncludeIssuePRAuthors, @@ -55,13 +53,11 @@ func (cfg githubSummarizer) ToGithubConfig() (github.Config, error) { func (cfg githubSummarizer) loadDefaultValues(v *viper.Viper) { v.SetDefault("github.host", "github.com") - v.SetDefault("github.issues-require-linked-prs", false) - v.SetDefault("github.include-issue-pr-authors", true) v.SetDefault("github.consider-pr-merge-commits", true) - v.SetDefault("github.include-prs", true) + v.SetDefault("github.include-issue-pr-authors", true) v.SetDefault("github.include-issues", true) v.SetDefault("github.include-prs", true) - v.SetDefault("github.include-unlabeled-prs", true) + v.SetDefault("github.issues-require-linked-prs", false) v.SetDefault("github.exclude-labels", []string{"duplicate", "question", "invalid", "wontfix", "wont-fix", "release-ignore", "changelog-ignore", "ignore"}) v.SetDefault("github.changes", []githubChange{ { @@ -100,11 +96,5 @@ func (cfg githubSummarizer) loadDefaultValues(v *viper.Viper) { Labels: []string{"deprecated"}, SemVerKind: change.SemVerMinor.String(), }, - { - Type: change.UnlabeledPRs, - Title: "Unlabeled PRs", - Labels: []string{""}, - SemVerKind: change.SemVerMinor.String(), - }, }) } From 24849d5ddf51dff930a80a81994816bbc18071ae Mon Sep 17 00:00:00 2001 From: Keith Zantow Date: Mon, 23 Jan 2023 20:34:25 -0500 Subject: [PATCH 3/8] chore: refactor for more alignment with filtering between prs and issues Signed-off-by: Keith Zantow --- .../release/releasers/github/gh_issue.go | 7 ++ .../release/releasers/github/summarizer.go | 64 +++++++++---------- internal/git/remote.go | 2 +- 3 files changed, 38 insertions(+), 35 deletions(-) diff --git a/chronicle/release/releasers/github/gh_issue.go b/chronicle/release/releasers/github/gh_issue.go index 43d9583..27d454b 100644 --- a/chronicle/release/releasers/github/gh_issue.go +++ b/chronicle/release/releasers/github/gh_issue.go @@ -117,6 +117,13 @@ func issuesWithoutLabel(labels ...string) issueFilter { } } +func issuesWithChangeTypes(config Config) issueFilter { + return func(issue ghIssue) bool { + changeTypes := config.ChangeTypesByLabel.ChangeTypes(issue.Labels...) + return len(changeTypes) > 0 + } +} + // nolint:funlen func fetchClosedIssues(user, repo string) ([]ghIssue, error) { src := oauth2.StaticTokenSource( diff --git a/chronicle/release/releasers/github/summarizer.go b/chronicle/release/releasers/github/summarizer.go index 13873f6..b627557 100644 --- a/chronicle/release/releasers/github/summarizer.go +++ b/chronicle/release/releasers/github/summarizer.go @@ -162,7 +162,8 @@ func (s *Summarizer) Changes(sinceRef, untilRef string) ([]change.Change, error) // githubs ontology has PRs as the source of truth for issue linking. Linked PR information // is not available on the issue itself. extractedIssues := issuesExtractedFromPRs(s.config, allMergedPRs, sinceTag, untilTag, includeCommits) - changes = append(changes, createChangesFromIssues(s.config, allMergedPRs, extractedIssues, filterIssuesByChangeTypes)...) + extractedIssues = filterIssues(extractedIssues, issuesWithChangeTypes(s.config)) + changes = append(changes, createChangesFromIssues(s.config, allMergedPRs, extractedIssues)...) } } @@ -284,15 +285,12 @@ func logPRs(prs []ghPullRequest) { func changesFromIssues(config Config, allMergedPRs []ghPullRequest, allClosedIssues []ghIssue, sinceTag, untilTag *git.Tag) []change.Change { filteredIssues := filterIssues(allClosedIssues, standardIssueFilters(config, sinceTag, untilTag)...) + filteredIssues = filterIssues(filteredIssues, issuesWithChangeTypes(config)) + log.Debugf("issues contributing to changelog: %d", len(filteredIssues)) logIssues(filteredIssues) - return createChangesFromIssues(config, allMergedPRs, filteredIssues, filterIssuesByChangeTypes) -} - -func filterIssuesByChangeTypes(config Config, issue ghIssue) bool { - changeTypes := config.ChangeTypesByLabel.ChangeTypes(issue.Labels...) - return len(changeTypes) > 0 + return createChangesFromIssues(config, allMergedPRs, filteredIssues) } func logIssues(issues []ghIssue) { @@ -305,40 +303,38 @@ func logIssues(issues []ghIssue) { } } -func createChangesFromIssues(config Config, allMergedPRs []ghPullRequest, issues []ghIssue, filter func(Config, ghIssue) bool) (changes []change.Change) { +func createChangesFromIssues(config Config, allMergedPRs []ghPullRequest, issues []ghIssue) (changes []change.Change) { for _, issue := range issues { - if filter(config, issue) { - changeTypes := config.ChangeTypesByLabel.ChangeTypes(issue.Labels...) + changeTypes := config.ChangeTypesByLabel.ChangeTypes(issue.Labels...) - references := []change.Reference{ - { - Text: fmt.Sprintf("Issue #%d", issue.Number), - URL: issue.URL, - }, - } + references := []change.Reference{ + { + Text: fmt.Sprintf("Issue #%d", issue.Number), + URL: issue.URL, + }, + } - if config.IncludeIssuePRAuthors { - for _, pr := range allMergedPRs { - for _, linkedIssue := range pr.LinkedIssues { - if linkedIssue.URL == issue.URL { - references = append(references, change.Reference{ - Text: pr.Author, - URL: fmt.Sprintf("https://%s/%s", config.Host, pr.Author), - }) - } + if config.IncludeIssuePRAuthors { + for _, pr := range allMergedPRs { + for _, linkedIssue := range pr.LinkedIssues { + if linkedIssue.URL == issue.URL { + references = append(references, change.Reference{ + Text: pr.Author, + URL: fmt.Sprintf("https://%s/%s", config.Host, pr.Author), + }) } } } - - changes = append(changes, change.Change{ - Text: issue.Title, - ChangeTypes: changeTypes, - Timestamp: issue.ClosedAt, - References: references, - EntryType: "githubIssue", - Entry: issue, - }) } + + changes = append(changes, change.Change{ + Text: issue.Title, + ChangeTypes: changeTypes, + Timestamp: issue.ClosedAt, + References: references, + EntryType: "githubIssue", + Entry: issue, + }) } return changes } diff --git a/internal/git/remote.go b/internal/git/remote.go index 2b834b7..9522470 100644 --- a/internal/git/remote.go +++ b/internal/git/remote.go @@ -10,7 +10,7 @@ import ( "github.com/anchore/chronicle/internal" ) -var remotePattern = regexp.MustCompile(`\[remote "origin"]\s*\n\s*url\s*=\s*(?P[^\s]+)\s+`) +var remotePattern = regexp.MustCompile(`\[remote\s*"origin"]\s*\n\s*url\s*=\s*(?P[^\s]+)\s+`) // TODO: can't use r.Config for same validation reasons func RemoteURL(p string) (string, error) { From 0944f8c9faf0a78c7a68e94e85a7d2a6dd918eac Mon Sep 17 00:00:00 2001 From: Keith Zantow Date: Mon, 23 Jan 2023 21:51:04 -0500 Subject: [PATCH 4/8] chore: undo refactoring that should be associated with unlabeled fields Signed-off-by: Keith Zantow --- .../release/releasers/github/gh_issue.go | 7 - .../releasers/github/gh_pull_request.go | 7 - .../release/releasers/github/summarizer.go | 127 +++++++++--------- internal/config/github.go | 2 +- 4 files changed, 61 insertions(+), 82 deletions(-) diff --git a/chronicle/release/releasers/github/gh_issue.go b/chronicle/release/releasers/github/gh_issue.go index 27d454b..43d9583 100644 --- a/chronicle/release/releasers/github/gh_issue.go +++ b/chronicle/release/releasers/github/gh_issue.go @@ -117,13 +117,6 @@ func issuesWithoutLabel(labels ...string) issueFilter { } } -func issuesWithChangeTypes(config Config) issueFilter { - return func(issue ghIssue) bool { - changeTypes := config.ChangeTypesByLabel.ChangeTypes(issue.Labels...) - return len(changeTypes) > 0 - } -} - // nolint:funlen func fetchClosedIssues(user, repo string) ([]ghIssue, error) { src := oauth2.StaticTokenSource( diff --git a/chronicle/release/releasers/github/gh_pull_request.go b/chronicle/release/releasers/github/gh_pull_request.go index e476ce9..d48a4c0 100644 --- a/chronicle/release/releasers/github/gh_pull_request.go +++ b/chronicle/release/releasers/github/gh_pull_request.go @@ -161,13 +161,6 @@ func prsWithLabel(labels ...string) prFilter { } } -func prsWithChangeTypes(config Config) prFilter { - return func(pr ghPullRequest) bool { - changeTypes := config.ChangeTypesByLabel.ChangeTypes(pr.Labels...) - return len(changeTypes) > 0 - } -} - func prsWithoutLabel(labels ...string) prFilter { return func(pr ghPullRequest) bool { for _, targetLabel := range labels { diff --git a/chronicle/release/releasers/github/summarizer.go b/chronicle/release/releasers/github/summarizer.go index b627557..ede1dbf 100644 --- a/chronicle/release/releasers/github/summarizer.go +++ b/chronicle/release/releasers/github/summarizer.go @@ -24,9 +24,9 @@ type Config struct { IncludeIssuePRAuthors bool IncludeIssues bool IncludePRs bool - IssuesRequireLinkedPR bool ExcludeLabels []string ChangeTypesByLabel change.TypeSet + IssuesRequireLinkedPR bool ConsiderPRMergeCommits bool } @@ -153,29 +153,27 @@ func (s *Summarizer) Changes(sinceRef, untilRef string) ([]change.Change, error) log.Debugf("total merged PRs discovered: %d", len(allMergedPRs)) - if s.config.IncludePRs || (s.config.IssuesRequireLinkedPR && s.config.IncludeIssues) { - if s.config.IncludePRs { - changes = append(changes, changesFromStandardPRFilters(s.config, allMergedPRs, sinceTag, untilTag, includeCommits)...) - } - if s.config.IssuesRequireLinkedPR && s.config.IncludeIssues { + if s.config.IncludePRs { + changes = append(changes, changesFromPRs(s.config, allMergedPRs, sinceTag, untilTag, includeCommits)...) + } + + if s.config.IncludeIssues { + if s.config.IssuesRequireLinkedPR { // extract closed linked issues with closed PRs from the PR list. Why do this here? // githubs ontology has PRs as the source of truth for issue linking. Linked PR information // is not available on the issue itself. extractedIssues := issuesExtractedFromPRs(s.config, allMergedPRs, sinceTag, untilTag, includeCommits) - extractedIssues = filterIssues(extractedIssues, issuesWithChangeTypes(s.config)) changes = append(changes, createChangesFromIssues(s.config, allMergedPRs, extractedIssues)...) - } - } - - allClosedIssues, err := fetchClosedIssues(s.userName, s.repoName) - if err != nil { - return nil, err - } + } else { + allClosedIssues, err := fetchClosedIssues(s.userName, s.repoName) + if err != nil { + return nil, err + } - log.Debugf("total closed issues discovered: %d", len(allClosedIssues)) + log.Debugf("total closed issues discovered: %d", len(allClosedIssues)) - if s.config.IncludeIssues && !s.config.IssuesRequireLinkedPR { - changes = append(changes, changesFromIssues(s.config, allMergedPRs, allClosedIssues, sinceTag, untilTag)...) + changes = append(changes, changesFromIssues(s.config, allMergedPRs, allClosedIssues, sinceTag, untilTag)...) + } } return changes, nil @@ -236,38 +234,34 @@ func uniqueIssuesFromPRs(prs []ghPullRequest) []ghIssue { return issues } -func changesFromStandardPRFilters(config Config, allMergedPRs []ghPullRequest, sinceTag, untilTag *git.Tag, includeCommits []string) []change.Change { +func changesFromPRs(config Config, allMergedPRs []ghPullRequest, sinceTag, untilTag *git.Tag, includeCommits []string) []change.Change { includedPRs := applyStandardPRFilters(allMergedPRs, config, sinceTag, untilTag, includeCommits) - includedPRs, _ = filterPRs(includedPRs, prsWithChangeTypes(config)) - log.Debugf("PRs contributing to changelog: %d", len(includedPRs)) logPRs(includedPRs) - return changesFromPRs(config, includedPRs) -} - -func changesFromPRs(config Config, prs []ghPullRequest) []change.Change { var summaries []change.Change - for _, pr := range prs { + for _, pr := range includedPRs { changeTypes := config.ChangeTypesByLabel.ChangeTypes(pr.Labels...) - summaries = append(summaries, change.Change{ - Text: pr.Title, - ChangeTypes: changeTypes, - Timestamp: pr.MergedAt, - References: []change.Reference{ - { - Text: fmt.Sprintf("PR #%d", pr.Number), - URL: pr.URL, - }, - { - Text: pr.Author, - URL: fmt.Sprintf("https://%s/%s", config.Host, pr.Author), + if len(changeTypes) > 0 { + summaries = append(summaries, change.Change{ + Text: pr.Title, + ChangeTypes: changeTypes, + Timestamp: pr.MergedAt, + References: []change.Reference{ + { + Text: fmt.Sprintf("PR #%d", pr.Number), + URL: pr.URL, + }, + { + Text: pr.Author, + URL: fmt.Sprintf("https://%s/%s", config.Host, pr.Author), + }, }, - }, - EntryType: "githubPR", - Entry: pr, - }) + EntryType: "githubPR", + Entry: pr, + }) + } } return summaries } @@ -285,8 +279,6 @@ func logPRs(prs []ghPullRequest) { func changesFromIssues(config Config, allMergedPRs []ghPullRequest, allClosedIssues []ghIssue, sinceTag, untilTag *git.Tag) []change.Change { filteredIssues := filterIssues(allClosedIssues, standardIssueFilters(config, sinceTag, untilTag)...) - filteredIssues = filterIssues(filteredIssues, issuesWithChangeTypes(config)) - log.Debugf("issues contributing to changelog: %d", len(filteredIssues)) logIssues(filteredIssues) @@ -306,35 +298,36 @@ func logIssues(issues []ghIssue) { func createChangesFromIssues(config Config, allMergedPRs []ghPullRequest, issues []ghIssue) (changes []change.Change) { for _, issue := range issues { changeTypes := config.ChangeTypesByLabel.ChangeTypes(issue.Labels...) + if len(changeTypes) > 0 { + references := []change.Reference{ + { + Text: fmt.Sprintf("Issue #%d", issue.Number), + URL: issue.URL, + }, + } - references := []change.Reference{ - { - Text: fmt.Sprintf("Issue #%d", issue.Number), - URL: issue.URL, - }, - } - - if config.IncludeIssuePRAuthors { - for _, pr := range allMergedPRs { - for _, linkedIssue := range pr.LinkedIssues { - if linkedIssue.URL == issue.URL { - references = append(references, change.Reference{ - Text: pr.Author, - URL: fmt.Sprintf("https://%s/%s", config.Host, pr.Author), - }) + if config.IncludeIssuePRAuthors { + for _, pr := range allMergedPRs { + for _, linkedIssue := range pr.LinkedIssues { + if linkedIssue.URL == issue.URL { + references = append(references, change.Reference{ + Text: pr.Author, + URL: fmt.Sprintf("https://%s/%s", config.Host, pr.Author), + }) + } } } } - } - changes = append(changes, change.Change{ - Text: issue.Title, - ChangeTypes: changeTypes, - Timestamp: issue.ClosedAt, - References: references, - EntryType: "githubIssue", - Entry: issue, - }) + changes = append(changes, change.Change{ + Text: issue.Title, + ChangeTypes: changeTypes, + Timestamp: issue.ClosedAt, + References: references, + EntryType: "githubIssue", + Entry: issue, + }) + } } return changes } diff --git a/internal/config/github.go b/internal/config/github.go index 0584749..0683ba6 100644 --- a/internal/config/github.go +++ b/internal/config/github.go @@ -41,11 +41,11 @@ func (cfg githubSummarizer) ToGithubConfig() (github.Config, error) { } return github.Config{ Host: cfg.Host, + IncludeIssuePRAuthors: cfg.IncludeIssuePRAuthors, IncludeIssues: cfg.IncludeIssues, IncludePRs: cfg.IncludePRs, ExcludeLabels: cfg.ExcludeLabels, IssuesRequireLinkedPR: cfg.IssuesRequireLinkedPR, - IncludeIssuePRAuthors: cfg.IncludeIssuePRAuthors, ConsiderPRMergeCommits: cfg.ConsiderPRMergeCommits, ChangeTypesByLabel: typeSet, }, nil From cc7d5acfb167fb8d2a2033c677c93e21aeadb696 Mon Sep 17 00:00:00 2001 From: Keith Zantow Date: Tue, 24 Jan 2023 00:30:57 -0500 Subject: [PATCH 5/8] chore: add test for createChangesFromIssues Signed-off-by: Keith Zantow --- .../releasers/github/summarizer_test.go | 138 ++++++++++++++++++ 1 file changed, 138 insertions(+) diff --git a/chronicle/release/releasers/github/summarizer_test.go b/chronicle/release/releasers/github/summarizer_test.go index 9122aad..3070851 100644 --- a/chronicle/release/releasers/github/summarizer_test.go +++ b/chronicle/release/releasers/github/summarizer_test.go @@ -1,10 +1,13 @@ package github import ( + "encoding/json" + "reflect" "testing" "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/anchore/chronicle/chronicle/release/change" "github.com/anchore/chronicle/internal/git" @@ -768,3 +771,138 @@ func Test_changesFromIssuesExtractedFromPRs(t *testing.T) { }) } } + +func Test_createChangesFromIssues(t *testing.T) { + timeStart := time.Date(2021, time.September, 16, 19, 34, 0, 0, time.UTC) + + patch := change.NewType("patch", change.SemVerPatch) + + changeTypeSet := change.TypeSet{ + "bug": patch, + } + + issue1 := ghIssue{ + Title: "Issue 1", + Number: 1, + URL: "issue-1-url", + ClosedAt: timeStart, + Labels: []string{"bug"}, + } + + issue2 := ghIssue{ + Title: "Issue 2", + Number: 2, + URL: "issue-2-url", + ClosedAt: timeStart, + Labels: []string{"bug"}, + } + + prWithLinkedIssues := ghPullRequest{ + Title: "pr with linked issues", + MergedAt: timeStart, + Number: 3, + Labels: []string{"bug"}, + Author: "some-author-1", + URL: "some-url-1", + LinkedIssues: []ghIssue{ + issue1, + }, + } + + prWithLinkedIssues2 := ghPullRequest{ + Title: "pr with linked issues 2", + MergedAt: timeStart, + Number: 4, + Labels: []string{"another-label"}, + Author: "some-author-2", + URL: "some-url-2", + LinkedIssues: []ghIssue{ + issue2, + }, + } + + prWithoutLinkedIssues := ghPullRequest{ + MergedAt: timeStart, + Title: "pr without linked issues", + Number: 6, + Author: "some-author", + URL: "some-url", + } + + tests := []struct { + name string + config Config + inputPrs []ghPullRequest + issues []ghIssue + expectedChanges []change.Change + }{ + { + name: "includes author for issues", + config: Config{ + IncludeIssuePRAuthors: true, + ChangeTypesByLabel: changeTypeSet, + Host: "some-host", + }, + inputPrs: []ghPullRequest{ + prWithLinkedIssues, + prWithLinkedIssues2, + prWithoutLinkedIssues, + }, + issues: []ghIssue{ + issue1, + issue2, + }, + expectedChanges: []change.Change{ + { + Text: "Issue 1", + ChangeTypes: []change.Type{patch}, + Timestamp: timeStart, + References: []change.Reference{ + { + Text: "Issue #1", + URL: "issue-1-url", + }, + { + Text: "some-author-1", + URL: "https://some-host/some-author-1", + }, + }, + EntryType: "githubIssue", + Entry: issue1, + }, + { + Text: "Issue 2", + ChangeTypes: []change.Type{patch}, + Timestamp: timeStart, + References: []change.Reference{ + { + Text: "Issue #2", + URL: "issue-2-url", + }, + { + Text: "some-author-2", + URL: "https://some-host/some-author-2", + }, + }, + EntryType: "githubIssue", + Entry: issue2, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + changes := createChangesFromIssues(tt.config, tt.inputPrs, tt.issues) + if !reflect.DeepEqual(tt.expectedChanges, changes) { + // print out a JSON diff + toJson := func(changes []change.Change) string { + out, err := json.Marshal(changes) + require.NoError(t, err) + return string(out) + } + assert.JSONEq(t, toJson(tt.expectedChanges), toJson(changes)) + } + }) + } +} From 3cf6aa10f954eec9d691ad0ae8a558f3964e224d Mon Sep 17 00:00:00 2001 From: Keith Zantow Date: Tue, 24 Jan 2023 00:49:53 -0500 Subject: [PATCH 6/8] chore: update remote url extraction test Signed-off-by: Keith Zantow --- internal/git/test-fixtures/Makefile | 5 ++++- internal/git/test-fixtures/create-remote-repo.sh | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/git/test-fixtures/Makefile b/internal/git/test-fixtures/Makefile index cac737d..3d46536 100644 --- a/internal/git/test-fixtures/Makefile +++ b/internal/git/test-fixtures/Makefile @@ -12,4 +12,7 @@ repos/commit-in-repo: ./create-commit-in-repo.sh repos/tag-range-repo: - ./create-tag-range-repo.sh \ No newline at end of file + ./create-tag-range-repo.sh + +clean: + rm -rf repos/remote-repo repos/tagged-repo repos/commit-in-repo repos/tag-range-repo diff --git a/internal/git/test-fixtures/create-remote-repo.sh b/internal/git/test-fixtures/create-remote-repo.sh index 467b08e..22ed02f 100755 --- a/internal/git/test-fixtures/create-remote-repo.sh +++ b/internal/git/test-fixtures/create-remote-repo.sh @@ -19,3 +19,4 @@ git config --local user.name "nope" trap 'popd' EXIT git remote add origin git@github.com:wagoodman/count-goober.git +git remote add upstream git@github.com:upstream/count-goober.git From c43a7ea38b92b8a0f3d69dbec4db3c6a9e36f0a2 Mon Sep 17 00:00:00 2001 From: Keith Zantow Date: Tue, 24 Jan 2023 02:08:23 -0500 Subject: [PATCH 7/8] chore: undo unnecessary changes Signed-off-by: Keith Zantow --- internal/config/github.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/config/github.go b/internal/config/github.go index 0683ba6..db9451b 100644 --- a/internal/config/github.go +++ b/internal/config/github.go @@ -13,8 +13,8 @@ type githubSummarizer struct { Host string `yaml:"host" json:"host" mapstructure:"host"` ExcludeLabels []string `yaml:"exclude-labels" json:"exclude-labels" mapstructure:"exclude-labels"` IncludeIssuePRAuthors bool `yaml:"include-issue-pr-authors" json:"include-issue-pr-authors" mapstructure:"include-issue-pr-authors"` - IncludeIssues bool `yaml:"include-issues" json:"include-issues" mapstructure:"include-issues"` IncludePRs bool `yaml:"include-prs" json:"include-prs" mapstructure:"include-prs"` + IncludeIssues bool `yaml:"include-issues" json:"include-issues" mapstructure:"include-issues"` IssuesRequireLinkedPR bool `yaml:"issues-require-linked-prs" json:"issues-require-linked-prs" mapstructure:"issues-require-linked-prs"` ConsiderPRMergeCommits bool `yaml:"consider-pr-merge-commits" json:"consider-pr-merge-commits" mapstructure:"consider-pr-merge-commits"` Changes []githubChange `yaml:"changes" json:"changes" mapstructure:"changes"` @@ -53,11 +53,11 @@ func (cfg githubSummarizer) ToGithubConfig() (github.Config, error) { func (cfg githubSummarizer) loadDefaultValues(v *viper.Viper) { v.SetDefault("github.host", "github.com") + v.SetDefault("github.issues-require-linked-prs", false) v.SetDefault("github.consider-pr-merge-commits", true) + v.SetDefault("github.include-prs", true) v.SetDefault("github.include-issue-pr-authors", true) v.SetDefault("github.include-issues", true) - v.SetDefault("github.include-prs", true) - v.SetDefault("github.issues-require-linked-prs", false) v.SetDefault("github.exclude-labels", []string{"duplicate", "question", "invalid", "wontfix", "wont-fix", "release-ignore", "changelog-ignore", "ignore"}) v.SetDefault("github.changes", []githubChange{ { From 2a61d361ccfee6b979af662a9c08c1ea3f9f825e Mon Sep 17 00:00:00 2001 From: Keith Zantow Date: Tue, 24 Jan 2023 02:15:07 -0500 Subject: [PATCH 8/8] chore: skip empty authors Signed-off-by: Keith Zantow --- chronicle/release/releasers/github/summarizer.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/chronicle/release/releasers/github/summarizer.go b/chronicle/release/releasers/github/summarizer.go index ede1dbf..f4edd75 100644 --- a/chronicle/release/releasers/github/summarizer.go +++ b/chronicle/release/releasers/github/summarizer.go @@ -308,6 +308,9 @@ func createChangesFromIssues(config Config, allMergedPRs []ghPullRequest, issues if config.IncludeIssuePRAuthors { for _, pr := range allMergedPRs { + if pr.Author == "" { + continue + } for _, linkedIssue := range pr.LinkedIssues { if linkedIssue.URL == issue.URL { references = append(references, change.Reference{