diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 62650205f686..2a5a00273f70 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -255,6 +255,7 @@ /pkg/cmd/gossipsim/ @cockroachdb/kv-prs /pkg/cmd/import-tools/ @cockroachdb/dev-inf /pkg/cmd/internal/issues/ @cockroachdb/test-eng +/pkg/cmd/lint-epic-issue-refs/ @cockroachdb/dev-inf /pkg/cmd/mirror/ @cockroachdb/dev-inf /pkg/cmd/prereqs/ @cockroachdb/dev-inf /pkg/cmd/protoc-gen-gogoroach/ @cockroachdb/dev-inf diff --git a/.github/workflows/pr-epic-issue-ref-lint.yml b/.github/workflows/pr-epic-issue-ref-lint.yml new file mode 100644 index 000000000000..408b7a45163c --- /dev/null +++ b/.github/workflows/pr-epic-issue-ref-lint.yml @@ -0,0 +1,35 @@ +name: Epic + Issue Ref Linter for PR Body or Commit Messages + +on: + pull_request: + types: [opened, reopened, synchronize, edited] + +jobs: + linter: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v2 + + - name: Bazel cache + id: bazel-cache + uses: actions/cache@v2 + with: + path: | + ~/.cache/bazel + key: ${{ runner.os }}-bazel-cache + + - name: Install bazelisk + run: | + curl -fsSL https://github.com/bazelbuild/bazelisk/releases/download/v1.10.1/bazelisk-linux-amd64 > /tmp/bazelisk + sha256sum -c - < 0 { + for _, x := range allMatches { + matches := secondMatch.FindAllString(x, -1) + for _, match := range matches { + ids[match]++ + } + } + } + return ids +} + +func extractFixIssueIDs(message string) map[string]int { + return extractStringsFromMessage(message, fixIssueRefRE, githubJiraIssueRefRE) +} + +func extractInformIssueIDs(message string) map[string]int { + return extractStringsFromMessage(message, informIssueRefRE, githubJiraIssueRefRE) +} + +func extractEpicIDs(message string) map[string]int { + return extractStringsFromMessage(message, epicRefRE, jiraIssueRefRE) +} + +func extractEpicNone(message string) bool { + if allMatches := epicNoneRE.FindAllString(message, -1); len(allMatches) > 0 { + return true + } + return false +} + +func scanCommitsForEpicAndIssueRefs( + ctx context.Context, ghClient *github.Client, params *parameters, +) ([]commitInfo, error) { + commits := []*github.RepositoryCommit{} + for page := 1; page <= 3; page++ { + commitsInQuery, _, err := ghClient.PullRequests.ListCommits( + ctx, params.Org, params.Repo, params.PrNumber, &github.ListOptions{PerPage: 100, Page: page}, + ) + if err != nil { + return nil, err + } + commits = append(commits, commitsInQuery...) + if len(commitsInQuery) < 100 { + break + } + } + + var infos []commitInfo + for _, commit := range commits { + message := commit.GetCommit().GetMessage() + var info = commitInfo{ + epicRefs: extractEpicIDs(message), + epicNone: extractEpicNone(message), + issueCloseRefs: extractFixIssueIDs(message), + issueInformRefs: extractInformIssueIDs(message), + sha: commit.GetSHA(), + } + infos = append(infos, info) + } + + return infos, nil +} + +func commitShaURL(sha string, params *parameters) string { + return fmt.Sprintf("https://github.com/%s/%s/commit/%s", params.Org, params.Repo, sha) +} + +// checkForMissingRefs determines if the PR and its commits has the needed refs. +// When the PR body is missing a ref and one or more commits are missing a ref, tell the +// caller which commits are missing refs. +// +// Returns: +// - True if the check passed. False if it failed. +func checkForMissingRefs(prInfo prBodyInfo, commitInfos []commitInfo, params *parameters) bool { + // When the PR body has a ref, no refs are needed in individual commits + if len(prInfo.epicRefs)+len(prInfo.issueInformRefs)+len(prInfo.issueCloseRefs) > 0 || prInfo.epicNone { + return true + } + + commitsWithoutRefs := []string{} + for _, info := range commitInfos { + if len(info.epicRefs)+len(info.issueInformRefs)+len(info.issueCloseRefs) == 0 && !info.epicNone { + commitsWithoutRefs = append(commitsWithoutRefs, commitShaURL(info.sha, params)) + } + } + if len(commitsWithoutRefs) > 0 { + if len(commitsWithoutRefs) == len(commitInfos) { + fmt.Print("Error: The PR body or each commit in the PR should have an epic or issue ref but they don't\n\n") + } else { + fmt.Printf("Error: These commits should have an epic or issue ref but don't: %v\n\n", commitsWithoutRefs) + } + return false + } + return true +} + +// checkPrEpicsUsedInCommits checks that all epic references in the PR body are used in at least one +// commit message and that the individual commits note the epic with which they are associated. +// +// Returns: +// - True if the check passed. False if it failed. +func checkPrEpicsUsedInCommits( + prInfo prBodyInfo, commitInfos []commitInfo, params *parameters, +) bool { + if len(prInfo.epicRefs) < 2 { + return true + } + + passedCheck := true + printHeader := func() { + if !passedCheck { + return + } + fmt.Print( + "When multiple epics are referenced in the PR body, each commit of the PR should " + + "reference the epic(s) that commit is part of and all epics listed in the commit messages " + + "should be listed in the PR body. These unexpected cases were found:\n", + ) + passedCheck = false + } + + // Check PR body epics are all referenced from commit messages + for _, prEpic := range reflect.ValueOf(prInfo.epicRefs).MapKeys() { + found := false + for _, ci := range commitInfos { + if ci.epicRefs[prEpic.String()] > 0 { + found = true + } + } + if !found { + printHeader() + fmt.Printf( + "- Error: This epic was listed in the PR body but was not referenced in any commits: %s\n", + prEpic.String(), + ) + } + } + + // Check that all commit messages reference one of the PR epics + for _, ci := range commitInfos { + if ci.epicNone { + continue + } + if len(ci.epicRefs) == 0 { + printHeader() + fmt.Printf("- Error: This commit did not reference an epic but should: %#v\n", commitShaURL(ci.sha, params)) + } else { + for _, epicRef := range reflect.ValueOf(ci.epicRefs).MapKeys() { + if _, ok := prInfo.epicRefs[epicRef.String()]; !ok { + printHeader() + fmt.Printf( + "- Error: This commit references an epic that isn't referenced in the PR body. "+ + "epic_ref: %v, commit: %s\n", + epicRef, + commitShaURL(ci.sha, params), + ) + } + } + } + } + + if !passedCheck { + fmt.Println() + } + + return passedCheck +} + +// checkMultipleEpicsInCommits checks for commits that contain multiple epic references and adds a +// warning that it is not a common case and to check it is intended. +// +// Returns: +// - True if the check passed. False if it failed. +func checkMultipleEpicsInCommits(commitInfos []commitInfo, params *parameters) bool { + passedCheck := true + for _, ci := range commitInfos { + if len(ci.epicRefs) > 1 { + fmt.Printf( + "Warning: This commit references multiple epics. Normally a commit only references one epic. "+ + "Noting this to verify this commit relates to multiple epics. "+ + "epic_refs=[%v], commit: %s\n\n", + ci.epicRefs, + commitShaURL(ci.sha, params), + ) + passedCheck = false + } + } + + return passedCheck +} + +func lintEpicAndIssueRefs(ctx context.Context, ghClient *github.Client, params *parameters) error { + commitInfos, err := scanCommitsForEpicAndIssueRefs(ctx, ghClient, params) + if err != nil { + return err + } + + pr, _, err := ghClient.PullRequests.Get( + ctx, params.Org, params.Repo, params.PrNumber, + ) + if err != nil { + // nolint:errwrap + return fmt.Errorf("error getting pull requests from GitHub: %v", err) + } + prBody := pr.GetBody() + + // Skip checking multi-PR backports for now. Verifying them is complex and the source PRs + // have already been checked. + if isMultiPrBackport(prBody) { + // TODO(jamesl): handle backport PRs containing multiple source PRs differently + // How might they be handled? + // - check that all source PR bodies have epic or issue refs? + // - figure out which commits in this PR correspond to the commits in the source PRs and ... + // - check all source PRs have all the expected references a PR should have. Do something + // with that info, like maybe link it back to the commits in this PR and report on it + // somehow??? (This seems really heavy / not useful.) + // - don't check backports with multiple source PRs for the complex cases like + // "checkPrEpicsUsedInCommits"? + // - these checks are really for making it possible for the automated docs issue generation to + // get the right epic(s) applied to them. Maybe make this a super simple check and then add + // info to the generated docs issue stating it needs to be reviewed more. This case shouldn't + // happen too often. The majority of backports are for a single source PR. + fmt.Println("This is a multi-PR backport. Skipping checking it.") + return nil + } + + var prInfo = prBodyInfo{ + epicRefs: extractEpicIDs(prBody), + epicNone: extractEpicNone(prBody), + issueCloseRefs: extractFixIssueIDs(prBody), + issueInformRefs: extractInformIssueIDs(prBody), + prNumber: params.PrNumber, + } + + passedAllTests := checkForMissingRefs(prInfo, commitInfos, params) && + checkPrEpicsUsedInCommits(prInfo, commitInfos, params) && + checkMultipleEpicsInCommits(commitInfos, params) + + if !passedAllTests { + fmt.Println("For more information about issue and epic references, see: https://wiki.crdb.io/wiki/spaces/CRDB/pages/2009039063/") + return errors.New("one or more checks failed; see logs above") + } + + // TODO: + // - add some "try" messages for how they can fix the issue in question. + return nil +} + +func lintPR(params *parameters) error { + ctx := context.Background() + + client := github.NewClient(oauth2.NewClient(ctx, oauth2.StaticTokenSource( + &oauth2.Token{AccessToken: params.Token}, + ))) + + return lintEpicAndIssueRefs(ctx, client, params) +} diff --git a/pkg/cmd/lint-epic-issue-refs/lint_epic_issue_refs_test.go b/pkg/cmd/lint-epic-issue-refs/lint_epic_issue_refs_test.go new file mode 100644 index 000000000000..c3274010f6c4 --- /dev/null +++ b/pkg/cmd/lint-epic-issue-refs/lint_epic_issue_refs_test.go @@ -0,0 +1,468 @@ +// Copyright 2022 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. +package main + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestExtractFixIssuesIDs(t *testing.T) { + testCases := []struct { + message string + expected map[string]int + }{ + { + message: `workload: Fix folder name generation. + +Fixes #75200 #98922 +close #75201 +closed #592 +RESOLVE #5555 + +Release Notes: None`, + expected: map[string]int{"#75200": 1, "#98922": 1, "#75201": 1, "#592": 1, "#5555": 1}, + }, + { + message: `logictestccl: skip flaky TestCCLLogic/fakedist-metadata/partitioning_enum + +Informs #75227 +Epic CRDB-491 + +Release note (bug fix): Fixin the bug`, + expected: map[string]int{}, + }, + { + message: `lots of variations + +fixes #74932; we were reading from the replicas map without... +Closes #74889. +Resolves #74482, #74784 #65117 #79299. +Fix: #73834 +epic: CRDB-9234. +epic CRDB-235, CRDB-40192; DEVINF-392 +Fixed: #29833 example/repo#941 +see also: #9243 +informs: #912, #4729 #2911 cockroachdb/cockroach#2934 + +Release note (sql change): Import now checks readability...`, + expected: map[string]int{"#74932": 1, "#74889": 1, "#74482": 1, "#74784": 1, "#65117": 1, "#79299": 1, "#73834": 1, "#29833": 1, "example/repo#941": 1}, + }, + { + message: `lots of variations 2 + +Resolved: #4921 +This comes w/ support for Applie Silicon Macs. Closes #72829. +This doesn't completely fix #71901 in that some... + Fixes #491 +Resolves #71614, Resolves #71607 +Thereby, fixes #69765 +Informs #69765 (point 4). +Fixes #65200. The last remaining 21.1 version (V21_1) can be removed as + +Release note (build change): Upgrade to Go 1.17.6 +Release note (ops change): Added a metric +Release notes (enterprise change): Client certificates may...`, + expected: map[string]int{"#4921": 1, "#72829": 1, "#71901": 1, "#491": 1, "#71614": 1, "#71607": 1, "#69765": 1, "#65200": 1}, + }, + { + message: `testing JIRA tickets + +Resolved: doc-4321 +This fixes everything. Closes CC-1234. + Fixes CRDB-12345 +Resolves crdb-23456, Resolves DEVINFHD-12345 +Fixes #12345 +Release notes (sql change): Excellent sql change...`, + expected: map[string]int{"doc-4321": 1, "CC-1234": 1, "CRDB-12345": 1, "crdb-23456": 1, "DEVINFHD-12345": 1, "#12345": 1}, + }, + } + + for _, tc := range testCases { + t.Run(tc.message, func(t *testing.T) { + result := extractFixIssueIDs(tc.message) + assert.Equal(t, tc.expected, result) + }) + } +} + +func TestExtractInformIssueIDs(t *testing.T) { + testCases := []struct { + message string + expected map[string]int + }{ + { + message: `logictestccl: skip flaky TestCCLLogic/fakedist-metadata/partitioning_enum + +Informs #75227 +Epic CRDB-491 +Fix: #73834 + +Release note (bug fix): Fixin the bug`, + expected: map[string]int{"#75227": 1}, + }, + { + message: `lots of variations + +Fixed: #29833 example/repo#941 +see also: #9243 +informs: #912, #4729 #2911 cockroachdb/cockroach#2934 +Informs #69765 (point 4). +This informs #59293 with these additions: + +Release note (sql change): Import now checks readability...`, + expected: map[string]int{"#9243": 1, "#912": 1, "#4729": 1, "#2911": 1, "cockroachdb/cockroach#2934": 1, "#69765": 1, "#59293": 1}, + }, + { + message: `testing JIRA keys with varying cases + +Fixed: CRDB-12345 example/repo#941 +informs: doc-1234, crdb-24680 +Informs DEVINF-123, #69765 + +Release note (sql change): Something something something...`, + expected: map[string]int{"doc-1234": 1, "DEVINF-123": 1, "#69765": 1, "crdb-24680": 1}, + }, + } + + for _, tc := range testCases { + t.Run(tc.message, func(t *testing.T) { + result := extractInformIssueIDs(tc.message) + assert.Equal(t, tc.expected, result) + }) + } +} + +func TestExtractEpicIDs(t *testing.T) { + testCases := []struct { + message string + expected map[string]int + }{ + { + message: `logictestccl: skip flaky TestCCLLogic/fakedist-metadata/partitioning_enum + +Informs #75227 +Epic CRDB-491 +Fix: #73834 + +Release note (bug fix): Fixin the bug`, + expected: map[string]int{"CRDB-491": 1}, + }, + { + message: `lots of variations + +epic: CRDB-9234. +epic CRDB-235, CRDB-40192; DEVINF-392 + +Release note (sql change): Import now checks readability...`, + expected: map[string]int{"CRDB-9234": 1, "CRDB-235": 1, "CRDB-40192": 1, "DEVINF-392": 1}, + }, + } + + for _, tc := range testCases { + t.Run(tc.message, func(t *testing.T) { + result := extractEpicIDs(tc.message) + assert.Equal(t, tc.expected, result) + }) + } +} + +func TestExtractEpicNone(t *testing.T) { + testCases := []struct { + message string + expected bool + }{ + { + message: `logictestccl: skip flaky TestCCLLogic/fakedist-metadata/partitioning_enum + +Epic CRDB-491 +Fix: #73834 + +Release note (bug fix): Fixin the bug`, + expected: false, + }, + { + message: `lots of variations + +epic: None + +Release note (sql change): Import now checks readability...`, + expected: true, + }, + { + message: `another test + +Epic nONE + +Release note (sql change): Import now checks readability...`, + expected: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.message, func(t *testing.T) { + result := extractEpicNone(tc.message) + assert.Equal(t, tc.expected, result) + }) + } +} + +func TestCheckForMissingRefs(t *testing.T) { + testCases := []struct { + message string + prInfo prBodyInfo + commitInfos []commitInfo + expected bool + }{ + { + message: "No refs set anywhere", + prInfo: prBodyInfo{}, + commitInfos: []commitInfo{{sha: "d53a4c0"}}, + expected: false, + }, + { + message: "One commit without refs", + prInfo: prBodyInfo{}, + commitInfos: []commitInfo{{sha: "d53a4c0"}, {sha: "128e2038", epicNone: true}}, + expected: false, + }, + { + message: "Epic ref in PR body", + prInfo: prBodyInfo{epicRefs: map[string]int{"CRDB-4123": 1}}, + commitInfos: []commitInfo{{}}, + expected: true, + }, + { + message: "Epic None set in PR body", + prInfo: prBodyInfo{epicNone: true}, + commitInfos: []commitInfo{{}}, + expected: true, + }, + { + message: "Close ref set in PR body", + prInfo: prBodyInfo{issueCloseRefs: map[string]int{"#942": 1}}, + commitInfos: []commitInfo{{}}, + expected: true, + }, + { + message: "Inform ref set in PR body", + prInfo: prBodyInfo{issueInformRefs: map[string]int{"#294": 1}}, + commitInfos: []commitInfo{{}}, + expected: true, + }, + { + message: "Epic set in commit", + prInfo: prBodyInfo{}, + commitInfos: []commitInfo{{sha: "128e2038", epicRefs: map[string]int{"CRDB-3919": 1}}}, + expected: true, + }, + { + message: "Epic None set in commit", + prInfo: prBodyInfo{}, + commitInfos: []commitInfo{{sha: "128e2038", epicNone: true}}, + expected: true, + }, + { + message: "Inform refs set in commit", + prInfo: prBodyInfo{}, + commitInfos: []commitInfo{{sha: "128e2038", issueInformRefs: map[string]int{"#491": 1}}}, + expected: true, + }, + { + message: "Close refs set in commit", + prInfo: prBodyInfo{}, + commitInfos: []commitInfo{{sha: "128e2038", issueCloseRefs: map[string]int{"#94712": 1}}}, + expected: true, + }, + } + + params := parameters{Repo: "cockroach", Org: "cockroachdb"} + + for _, tc := range testCases { + t.Run(tc.message, func(t *testing.T) { + result := checkForMissingRefs(tc.prInfo, tc.commitInfos, ¶ms) + assert.Equal(t, tc.expected, result) + }) + } +} + +func TestCheckPrEpicsUsedInCommits(t *testing.T) { + testCases := []struct { + message string + prInfo prBodyInfo + commitInfos []commitInfo + expected bool + }{ + // Failing checks + { + message: "Only one epic used in a commit", + prInfo: prBodyInfo{epicRefs: map[string]int{"CRDB-4123": 1, "CRDB-1929": 1}}, + commitInfos: []commitInfo{{epicRefs: map[string]int{"CRDB-4123": 1}}}, + expected: false, + }, + { + message: "An epic used in a commit is not used in the PR body", + prInfo: prBodyInfo{epicRefs: map[string]int{"CRDB-4123": 1, "CRDB-1929": 1}}, + commitInfos: []commitInfo{ + {epicRefs: map[string]int{"CRDB-4123": 1}}, + {epicRefs: map[string]int{"CRDB-1929": 1}}, + {epicRefs: map[string]int{"CRDB-94": 1}}, + }, + expected: false, + }, + { + message: "Commit without an epic", + prInfo: prBodyInfo{epicRefs: map[string]int{"CRDB-4123": 1, "CRDB-1929": 1}}, + commitInfos: []commitInfo{ + {epicRefs: map[string]int{"CRDB-4123": 1}}, + {epicRefs: map[string]int{"CRDB-1929": 1}}, + {}, + }, + expected: false, + }, + // Succeeding checks + { + message: "No epics in PR body", + prInfo: prBodyInfo{epicRefs: map[string]int{}}, + commitInfos: []commitInfo{{sha: "d53a4c0"}}, + expected: true, + }, + { + message: "One epic in PR body", + prInfo: prBodyInfo{epicRefs: map[string]int{"CRDB-48190": 1}}, + commitInfos: []commitInfo{{sha: "d53a4c0"}}, + expected: true, + }, + { + message: "Epics used in the same commit", + prInfo: prBodyInfo{epicRefs: map[string]int{"CRDB-4123": 1, "CRDB-1929": 1}}, + commitInfos: []commitInfo{{epicRefs: map[string]int{"CRDB-4123": 1, "CRDB-1929": 1}}}, + expected: true, + }, + { + message: "Epics used in different commits", + prInfo: prBodyInfo{epicRefs: map[string]int{"CRDB-4123": 1, "CRDB-1929": 1}}, + commitInfos: []commitInfo{ + {epicRefs: map[string]int{"CRDB-4123": 1}}, + {epicRefs: map[string]int{"CRDB-1929": 1}}, + }, + expected: true, + }, + { + message: "Epics used in multiple different commits", + prInfo: prBodyInfo{epicRefs: map[string]int{"CRDB-4123": 1, "CRDB-1929": 1}}, + commitInfos: []commitInfo{ + {epicRefs: map[string]int{"CRDB-4123": 1}}, + {epicRefs: map[string]int{"CRDB-1929": 1}}, + {epicRefs: map[string]int{"CRDB-1929": 1}}, + }, + expected: true, + }, + { + message: "Epics used in different commits plus an Epic none used", + prInfo: prBodyInfo{epicRefs: map[string]int{"CRDB-4123": 1, "CRDB-1929": 1}}, + commitInfos: []commitInfo{ + {epicRefs: map[string]int{"CRDB-4123": 1}}, + {epicRefs: map[string]int{"CRDB-1929": 1}}, + {epicNone: true}, + }, + expected: true, + }, + } + + params := parameters{Repo: "cockroach", Org: "cockroachdb"} + + for _, tc := range testCases { + t.Run(tc.message, func(t *testing.T) { + result := checkPrEpicsUsedInCommits(tc.prInfo, tc.commitInfos, ¶ms) + assert.Equal(t, tc.expected, result) + }) + } +} + +func TestCheckMultipleEpicsInCommits(t *testing.T) { + testCases := []struct { + message string + commitInfos []commitInfo + expected bool + }{ + { + message: "No epic used in a commit", + commitInfos: []commitInfo{{epicRefs: map[string]int{}}}, + expected: true, + }, + { + message: "One epic used in a commit", + commitInfos: []commitInfo{{epicRefs: map[string]int{"CRDB-4123": 1}}}, + expected: true, + }, + { + message: "Two epics used in a commit", + commitInfos: []commitInfo{{epicRefs: map[string]int{"CRDB-4123": 1, "CC-4589": 1}}}, + expected: false, + }, + { + message: "Multiple commits", + commitInfos: []commitInfo{ + {epicRefs: map[string]int{"CRDB-4123": 1}}, + {epicRefs: map[string]int{"CRDB-1929": 1}}, + {epicRefs: map[string]int{}}, + }, + expected: true, + }, + } + + params := parameters{Repo: "cockroach", Org: "cockroachdb"} + + for _, tc := range testCases { + t.Run(tc.message, func(t *testing.T) { + result := checkMultipleEpicsInCommits(tc.commitInfos, ¶ms) + assert.Equal(t, tc.expected, result) + }) + } +} + +func TestMultipPrBackport(t *testing.T) { + testCases := []struct { + message string + expected bool + }{ + { + message: "Backport 1/1 commits from #71039.", + expected: false, + }, + { + message: "Backport 2/2 commits from #79612", + expected: false, + }, + { + message: "Something else entirely", + expected: false, + }, + { + message: "Backport for 2 commits, 1/1 from #76516 and 1/1 from #79157.", + expected: true, + }, + { + message: `Backport: + * 1/1 commits from "sql: do not collect stats for virtual columns" (#68312) + * 1/1 commits from "sql: do not collect statistics on virtual columns" (#71105)`, + expected: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.message, func(t *testing.T) { + result := isMultiPrBackport(tc.message) + assert.Equal(t, tc.expected, result) + }) + } +} diff --git a/pkg/cmd/lint-epic-issue-refs/main.go b/pkg/cmd/lint-epic-issue-refs/main.go new file mode 100644 index 000000000000..6d43f74f81c5 --- /dev/null +++ b/pkg/cmd/lint-epic-issue-refs/main.go @@ -0,0 +1,93 @@ +// Copyright 2022 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +// Check that GitHub PR descriptions and commit messages contain the +// expected epic and issue references. +package main + +import ( + "errors" + "fmt" + "os" + "strconv" + + "github.com/spf13/cobra" +) + +var rootCmd = &cobra.Command{ + Use: "lint-epic-issue-refs PR_NUMBER", + Short: "Check the body and commit messages of the indicated PR for epic and issue refs", + Args: cobra.ExactArgs(1), + Run: func(_ *cobra.Command, args []string) { + params := defaultEnvParameters() + prNumber, err := parseArgs(args) + if err != nil { + fmt.Printf("%v\n", err) + os.Exit(1) + } + + params.PrNumber = prNumber + err = lintPR(params) + if err != nil { + fmt.Printf("%v\n", err) + os.Exit(1) + } + }, +} + +func main() { + if err := rootCmd.Execute(); err != nil { + os.Exit(1) + } +} + +func parseArgs(args []string) (int, error) { + if len(args) < 1 { + return 0, errors.New("no PR number specified") + } + if len(args) > 1 { + return 0, errors.New("one argument is required: a PR number") + } + prNumber, err := strconv.Atoi(args[0]) + if err != nil { + return 0, fmt.Errorf("invalid PR number: %w", err) + } + + return prNumber, nil +} + +type parameters struct { + Token string // GitHub API token + Org string + Repo string + PrNumber int +} + +func defaultEnvParameters() *parameters { + const ( + githubTokenEnv = "GITHUB_TOKEN" + githubOrgEnv = "GITHUB_ORG" + githubRepoEnv = "GITHUB_REPO" + ) + + return ¶meters{ + Token: maybeEnv(githubTokenEnv, ""), + Org: maybeEnv(githubOrgEnv, "cockroachdb"), + Repo: maybeEnv(githubRepoEnv, "cockroach"), + } +} + +func maybeEnv(envKey, defaultValue string) string { + v := os.Getenv(envKey) + if v == "" { + return defaultValue + } + return v +} diff --git a/pkg/cmd/roachtest/tests/cdc.go b/pkg/cmd/roachtest/tests/cdc.go index f793ebc929c3..949cd7dd4a17 100644 --- a/pkg/cmd/roachtest/tests/cdc.go +++ b/pkg/cmd/roachtest/tests/cdc.go @@ -417,7 +417,7 @@ func runCDCBank(ctx context.Context, t test.Test, c cluster.Cluster) { return errors.Wrap(err, "CREATE TABLE failed") } - fprintV, err := cdctest.NewFingerprintValidator(db, `bank.bank`, `fprint`, tc.partitions, 0, false) + fprintV, err := cdctest.NewFingerprintValidator(db, `bank.bank`, `fprint`, tc.partitions, 0) if err != nil { return errors.Wrap(err, "error creating validator") } diff --git a/pkg/cmd/roachtest/tests/mixed_version_cdc.go b/pkg/cmd/roachtest/tests/mixed_version_cdc.go index f1b439857454..13a919e2b132 100644 --- a/pkg/cmd/roachtest/tests/mixed_version_cdc.go +++ b/pkg/cmd/roachtest/tests/mixed_version_cdc.go @@ -12,6 +12,7 @@ package tests import ( "context" + gosql "database/sql" "fmt" "runtime" "strconv" @@ -46,6 +47,15 @@ var ( targetTable = "bank" timeout = 30 * time.Minute + // set a fixed number of operations to be performed by the + // workload. Since we are validating events emitted by the + // changefeed in this test (ValidateDuplicatedEvents() call), + // enforcing a maximum number of operations sets a boundary on the + // validator's memory usage. The current value represents enough + // operations to allow for the validator to receive the desired + // amount of resolved events at different points of the upgrade + // process while staying within the current timeout. + maxOps = 12000 ) func registerCDCMixedVersions(r registry.Registry) { @@ -78,9 +88,11 @@ type cdcMixedVersionTester struct { syncutil.Mutex C chan struct{} } - kafka kafkaManager - validator *cdctest.CountValidator - cleanup func() + crdbUpgrading syncutil.Mutex + kafka kafkaManager + validator *cdctest.CountValidator + workloadDone bool + cleanup func() } func newCDCMixedVersionTester( @@ -125,10 +137,12 @@ func (cmvt *cdcMixedVersionTester) installAndStartWorkload() versionStep { t.Status("installing and running workload") u.c.Run(ctx, cmvt.workloadNodes, "./workload init bank {pgurl:1}") cmvt.monitor.Go(func(ctx context.Context) error { + defer func() { cmvt.workloadDone = true }() return u.c.RunE( ctx, cmvt.workloadNodes, - fmt.Sprintf("./workload run bank {pgurl%s} --max-rate=10 --tolerate-errors", cmvt.crdbNodes), + fmt.Sprintf("./workload run bank {pgurl%s} --max-rate=10 --max-ops %d --tolerate-errors", + cmvt.crdbNodes, maxOps), ) }) } @@ -169,6 +183,15 @@ func (cmvt *cdcMixedVersionTester) waitForResolvedTimestamps() versionStep { } } +// waitForWorkload waits for the workload to finish +func (cmvt *cdcMixedVersionTester) waitForWorkload() versionStep { + return func(ctx context.Context, t test.Test, u *versionUpgradeTest) { + t.L().Printf("waiting for workload to finish...") + cmvt.monitor.Wait() + t.L().Printf("workload finished") + } +} + // setupVerifier creates a CDC validator to validate that a changefeed // created on the `target` table is able to re-create the table // somewhere else. It also verifies CDC's ordering guarantees. This @@ -196,20 +219,22 @@ func (cmvt *cdcMixedVersionTester) setupVerifier(node int) versionStep { t.Fatal(err) } - fprintV, err := cdctest.NewFingerprintValidator(db, tableName, `fprint`, consumer.partitions, 0, true) + getConn := func(node int) *gosql.DB { return u.conn(ctx, t, node) } + fprintV, err := cdctest.NewFingerprintValidator(db, tableName, `fprint`, consumer.partitions, 0) if err != nil { t.Fatal(err) } + fprintV.DBFunc(cmvt.cdcDBConn(getConn)).ValidateDuplicatedEvents() validators := cdctest.Validators{ cdctest.NewOrderValidator(tableName), fprintV, } cmvt.validator = cdctest.MakeCountValidator(validators) - for { + for !cmvt.workloadDone { m := consumer.Next(ctx) if m == nil { - t.L().Printf("end of changefeed") + t.Fatal("unexpected end of changefeed") return nil } @@ -233,6 +258,7 @@ func (cmvt *cdcMixedVersionTester) setupVerifier(node int) versionStep { cmvt.timestampResolved() } } + return nil }) } } @@ -248,6 +274,35 @@ func (cmvt *cdcMixedVersionTester) timestampResolved() { } } +// cdcDBConn is the wrapper passed to the FingerprintValidator. The +// goal is to ensure that database checks by the validator do not +// happen while we are running an upgrade. We used to retry database +// calls in the validator, but that logic adds complexity and does not +// help in testing the changefeed's correctness +func (cmvt *cdcMixedVersionTester) cdcDBConn( + getConn func(int) *gosql.DB, +) func(func(*gosql.DB) error) error { + return func(f func(*gosql.DB) error) error { + cmvt.crdbUpgrading.Lock() + defer cmvt.crdbUpgrading.Unlock() + + node := cmvt.crdbNodes.RandNode()[0] + return f(getConn(node)) + } +} + +// crdbUpgradeStep is a wrapper to steps that upgrade the cockroach +// binary running in the cluster. It makes sure we hold exclusive +// access to the `crdbUpgrading` lock while the upgrade is in process +func (cmvt *cdcMixedVersionTester) crdbUpgradeStep(step versionStep) versionStep { + return func(ctx context.Context, t test.Test, u *versionUpgradeTest) { + cmvt.crdbUpgrading.Lock() + defer cmvt.crdbUpgrading.Unlock() + + step(ctx, t, u) + } +} + // assertValid checks if the validator has found any issues at the // time the function is called. func (cmvt *cdcMixedVersionTester) assertValid() versionStep { @@ -316,7 +371,7 @@ func runCDCMixedVersions( tester.waitForResolvedTimestamps(), // Roll the nodes into the new version one by one in random order - binaryUpgradeStep(tester.crdbNodes, mainVersion), + tester.crdbUpgradeStep(binaryUpgradeStep(tester.crdbNodes, mainVersion)), // let the workload run in the new version for a while tester.waitForResolvedTimestamps(), @@ -324,19 +379,20 @@ func runCDCMixedVersions( // Roll back again, which ought to be fine because the cluster upgrade was // not finalized. - binaryUpgradeStep(tester.crdbNodes, predecessorVersion), + tester.crdbUpgradeStep(binaryUpgradeStep(tester.crdbNodes, predecessorVersion)), tester.waitForResolvedTimestamps(), tester.assertValid(), // Roll nodes forward and finalize upgrade. - binaryUpgradeStep(tester.crdbNodes, mainVersion), + tester.crdbUpgradeStep(binaryUpgradeStep(tester.crdbNodes, mainVersion)), // allow cluster version to update allowAutoUpgradeStep(sqlNode()), waitForUpgradeStep(tester.crdbNodes), tester.waitForResolvedTimestamps(), + tester.waitForWorkload(), tester.assertValid(), ).run(ctx, t) } diff --git a/pkg/sql/drop_role.go b/pkg/sql/drop_role.go index 948a537039bb..6271900d2a95 100644 --- a/pkg/sql/drop_role.go +++ b/pkg/sql/drop_role.go @@ -195,15 +195,16 @@ func (n *DropRoleNode) startExec(params runParams) error { if !descriptorIsVisible(schemaDesc, true /* allowAdding */) { continue } - // TODO(arul): Ideally this should be the fully qualified name of the schema, - // but at the time of writing there doesn't seem to be a clean way of doing - // this. if _, ok := userNames[schemaDesc.GetPrivileges().Owner()]; ok { + sn, err := getSchemaNameFromSchemaDescriptor(lCtx, schemaDesc) + if err != nil { + return err + } userNames[schemaDesc.GetPrivileges().Owner()] = append( userNames[schemaDesc.GetPrivileges().Owner()], objectAndType{ ObjectType: privilege.Schema, - ObjectName: schemaDesc.GetName(), + ObjectName: sn.String(), }) } @@ -301,7 +302,12 @@ func (n *DropRoleNode) startExec(params runParams) error { if dependentObjects[i].ObjectName != dependentObjects[j].ObjectName { return dependentObjects[i].ObjectName < dependentObjects[j].ObjectName } - + if dependentObjects[j].ErrorMessage == nil { + return false + } + if dependentObjects[i].ErrorMessage == nil { + return true + } return dependentObjects[i].ErrorMessage.Error() < dependentObjects[j].ErrorMessage.Error() }) var hints []string diff --git a/pkg/sql/logictest/testdata/logic_test/drop_user b/pkg/sql/logictest/testdata/logic_test/drop_user index 51758cbcbda6..ff9e7d7f71ea 100644 --- a/pkg/sql/logictest/testdata/logic_test/drop_user +++ b/pkg/sql/logictest/testdata/logic_test/drop_user @@ -177,3 +177,33 @@ INSERT INTO system.scheduled_jobs (schedule_name, owner, executor_type,execution statement error pq: cannot drop role/user user1; it owns 1 scheduled jobs. DROP USER user1 + +# Verify that schemas are fully qualified in the error message. +subtest same_schema_name + +statement ok +CREATE ROLE schema_owner + +statement ok +GRANT admin TO schema_owner + +statement ok +SET ROLE schema_owner + +statement ok +CREATE SCHEMA the_schema + +statement ok +USE defaultdb + +statement ok +CREATE SCHEMA the_schema + +statement ok +RESET ROLE; +RESET DATABASE + +statement error role schema_owner cannot be dropped because some objects depend on it\nowner of schema defaultdb.the_schema\nowner of schema test.the_schema +DROP ROLE schema_owner + +subtest end diff --git a/pkg/sql/logictest/testdata/logic_test/owner b/pkg/sql/logictest/testdata/logic_test/owner index 6583d2dc8db2..45a956185f9b 100644 --- a/pkg/sql/logictest/testdata/logic_test/owner +++ b/pkg/sql/logictest/testdata/logic_test/owner @@ -253,5 +253,5 @@ REVOKE ALL ON TABLE d.s.t FROM testuser; user testuser -statement error pq: role testuser cannot be dropped because some objects depend on it\nowner of database d\nowner of schema s +statement error pq: role testuser cannot be dropped because some objects depend on it\nowner of database d\nowner of schema d.s DROP ROLE testuser diff --git a/pkg/sql/resolver.go b/pkg/sql/resolver.go index b26d24f50753..df4077619406 100644 --- a/pkg/sql/resolver.go +++ b/pkg/sql/resolver.go @@ -1179,6 +1179,22 @@ func getTypeNameFromTypeDescriptor( return typeName, nil } +func getSchemaNameFromSchemaDescriptor( + l simpleSchemaResolver, sc catalog.SchemaDescriptor, +) (tree.ObjectNamePrefix, error) { + var scName tree.ObjectNamePrefix + db, err := l.getDatabaseByID(sc.GetParentID()) + if err != nil { + return scName, err + } + return tree.ObjectNamePrefix{ + CatalogName: tree.Name(db.GetName()), + SchemaName: tree.Name(sc.GetName()), + ExplicitCatalog: true, + ExplicitSchema: true, + }, nil +} + func getFunctionNameFromFunctionDescriptor( l simpleSchemaResolver, fn catalog.FunctionDescriptor, ) (tree.FunctionName, error) {