-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
roachtest: Refactor github posting to separate source file, add unit …
…test. No previous coverage exists for posting issues to github. This change moves the relevant code into a separate source file with associated test. External functions dealing with actual issue posting to github, and loading the TEAMS.yaml are now injected to facilitate easier testing. Release justification: test-only change Release note: none
- Loading branch information
Miral Gadani
committed
Sep 12, 2022
1 parent
ac3ba1d
commit d7427e5
Showing
4 changed files
with
370 additions
and
108 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,143 @@ | ||
// 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 ( | ||
"context" | ||
"fmt" | ||
"os" | ||
|
||
"github.com/cockroachdb/cockroach/pkg/cmd/internal/issues" | ||
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry" | ||
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test" | ||
"github.com/cockroachdb/cockroach/pkg/internal/team" | ||
"github.com/cockroachdb/cockroach/pkg/roachprod/logger" | ||
"github.com/cockroachdb/cockroach/pkg/roachprod/vm" | ||
) | ||
|
||
type githubIssues struct { | ||
disable bool | ||
l *logger.Logger | ||
cluster *clusterImpl | ||
vmCreateOpts *vm.CreateOpts | ||
issuePoster func(ctx context.Context, formatter issues.IssueFormatter, req issues.PostRequest) error | ||
teamLoader func() (team.Map, error) | ||
} | ||
|
||
func newGithubIssues( | ||
disable bool, c *clusterImpl, vmCreateOpts *vm.CreateOpts, l *logger.Logger, | ||
) *githubIssues { | ||
|
||
return &githubIssues{ | ||
disable: disable, | ||
vmCreateOpts: vmCreateOpts, | ||
cluster: c, | ||
l: l, | ||
issuePoster: issues.Post, | ||
teamLoader: team.DefaultLoadTeams, | ||
} | ||
} | ||
|
||
func roachtestPrefix(p string) string { | ||
return "ROACHTEST_" + p | ||
} | ||
|
||
func (g *githubIssues) shouldPost(t test.Test) bool { | ||
opts := issues.DefaultOptionsFromEnv() | ||
return !g.disable && opts.CanPost() && | ||
opts.IsReleaseBranch() && | ||
t.Spec().(*registry.TestSpec).Run != nil && | ||
// NB: check NodeCount > 0 to avoid posting issues from this pkg's unit tests. | ||
t.Spec().(*registry.TestSpec).Cluster.NodeCount > 0 | ||
} | ||
|
||
func (g *githubIssues) createPostRequest(t test.Test, message string) issues.PostRequest { | ||
var mention []string | ||
var projColID int | ||
|
||
teams, err := g.teamLoader() | ||
if err != nil { | ||
t.Fatalf("could not load teams: %v", err) | ||
} | ||
|
||
if sl, ok := teams.GetAliasesForPurpose(ownerToAlias(t.Spec().(*registry.TestSpec).Owner), team.PurposeRoachtest); ok { | ||
for _, alias := range sl { | ||
mention = append(mention, "@"+string(alias)) | ||
} | ||
projColID = teams[sl[0]].TriageColumnID | ||
} | ||
|
||
branch := os.Getenv("TC_BUILD_BRANCH") | ||
if branch == "" { | ||
branch = "<unknown branch>" | ||
} | ||
|
||
artifacts := fmt.Sprintf("/%s", t.Name()) | ||
|
||
// Issues posted from roachtest are identifiable as such and | ||
// they are also release blockers (this label may be removed | ||
// by a human upon closer investigation). | ||
spec := t.Spec().(*registry.TestSpec) | ||
labels := []string{"O-roachtest"} | ||
if !spec.NonReleaseBlocker { | ||
labels = append(labels, "release-blocker") | ||
} | ||
|
||
clusterParams := map[string]string{ | ||
roachtestPrefix("cloud"): spec.Cluster.Cloud, | ||
roachtestPrefix("cpu"): fmt.Sprintf("%d", spec.Cluster.CPUs), | ||
roachtestPrefix("ssd"): fmt.Sprintf("%d", spec.Cluster.SSDs), | ||
} | ||
|
||
// These params can be probabilistically set, so we pass them here to | ||
// show what their actual values are in the posted issue. | ||
if g.vmCreateOpts != nil { | ||
clusterParams[roachtestPrefix("fs")] = g.vmCreateOpts.SSDOpts.FileSystem | ||
clusterParams[roachtestPrefix("localSSD")] = fmt.Sprintf("%v", g.vmCreateOpts.SSDOpts.UseLocalSSD) | ||
} | ||
|
||
if g.cluster != nil { | ||
clusterParams[roachtestPrefix("encrypted")] = fmt.Sprintf("%v", g.cluster.encAtRest) | ||
} | ||
|
||
return issues.PostRequest{ | ||
MentionOnCreate: mention, | ||
ProjectColumnID: projColID, | ||
PackageName: "roachtest", | ||
TestName: t.Name(), | ||
Message: message, | ||
Artifacts: artifacts, | ||
ExtraLabels: labels, | ||
ExtraParams: clusterParams, | ||
HelpCommand: func(renderer *issues.Renderer) { | ||
issues.HelpCommandAsLink( | ||
"roachtest README", | ||
"https://github.com/cockroachdb/cockroach/blob/master/pkg/cmd/roachtest/README.md", | ||
)(renderer) | ||
issues.HelpCommandAsLink( | ||
"How To Investigate (internal)", | ||
"https://cockroachlabs.atlassian.net/l/c/SSSBr8c7", | ||
)(renderer) | ||
}, | ||
} | ||
} | ||
|
||
func (g *githubIssues) MaybePost(t test.Test, message string) error { | ||
if !g.shouldPost(t) { | ||
return nil | ||
} | ||
|
||
return g.issuePoster( | ||
context.Background(), | ||
issues.UnitTestFormatter, | ||
g.createPostRequest(t, message), | ||
) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,208 @@ | ||
// 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 ( | ||
"context" | ||
"strings" | ||
"testing" | ||
|
||
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster" | ||
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry" | ||
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/spec" | ||
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test" | ||
"github.com/cockroachdb/cockroach/pkg/internal/team" | ||
"github.com/cockroachdb/cockroach/pkg/roachprod/vm" | ||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/require" | ||
) | ||
|
||
var ( | ||
teamsYaml = `cockroachdb/unowned: | ||
aliases: | ||
cockroachdb/rfc-prs: other | ||
triage_column_id: 0` | ||
|
||
validTeamsFn = func() (team.Map, error) { return loadYamlTeams(teamsYaml) } | ||
invalidTeamsFn = func() (team.Map, error) { return loadYamlTeams("invalid yaml") } | ||
) | ||
|
||
func loadYamlTeams(yaml string) (team.Map, error) { | ||
return team.LoadTeams(strings.NewReader(yaml)) | ||
} | ||
|
||
func prefixAll(params map[string]string) map[string]string { | ||
updated := make(map[string]string) | ||
|
||
for k, v := range params { | ||
updated[roachtestPrefix(k)] = v | ||
} | ||
|
||
return updated | ||
} | ||
|
||
func TestShouldPost(t *testing.T) { | ||
testCases := []struct { | ||
disableIssues bool | ||
nodeCount int | ||
envGithubAPIToken string | ||
envTcBuildBranch string | ||
expected bool | ||
}{ | ||
/* Cases 1 - 4 verify that issues are not posted if any of on the relevant criteria checks fail */ | ||
// disable | ||
{true, 1, "token", "master", false}, | ||
// nodeCount | ||
{false, 0, "token", "master", false}, | ||
// apiToken | ||
{false, 1, "", "master", false}, | ||
// branch | ||
{false, 1, "token", "", false}, | ||
{false, 1, "token", "master", true}, | ||
} | ||
|
||
reg, _ := makeTestRegistry(spec.GCE, "", "", false) | ||
for _, c := range testCases { | ||
t.Setenv("GITHUB_API_TOKEN", c.envGithubAPIToken) | ||
t.Setenv("TC_BUILD_BRANCH", c.envTcBuildBranch) | ||
|
||
clusterSpec := reg.MakeClusterSpec(c.nodeCount) | ||
testSpec := ®istry.TestSpec{ | ||
Name: "githubPost", | ||
Owner: OwnerUnitTest, | ||
Cluster: clusterSpec, | ||
// `shouldPost` explicitly checks to ensure that the run function is defined | ||
Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {}, | ||
} | ||
|
||
ti := &testImpl{ | ||
spec: testSpec, | ||
l: nilLogger(), | ||
} | ||
|
||
github := &githubIssues{ | ||
disable: c.disableIssues, | ||
} | ||
|
||
require.Equal(t, c.expected, github.shouldPost(ti)) | ||
} | ||
} | ||
|
||
func TestCreatePostRequest(t *testing.T) { | ||
testCases := []struct { | ||
nonReleaseBlocker bool | ||
clusterCreationFailed bool | ||
loadTeamsFailed bool | ||
localSSD bool | ||
expectedPost bool | ||
expectedParams map[string]string | ||
}{ | ||
{true, false, false, false, true, | ||
prefixAll(map[string]string{ | ||
"cloud": "gce", | ||
"encrypted": "false", | ||
"fs": "ext4", | ||
"ssd": "0", | ||
"cpu": "4", | ||
"localSSD": "false", | ||
}), | ||
}, | ||
{true, false, false, true, true, | ||
prefixAll(map[string]string{ | ||
"cloud": "gce", | ||
"encrypted": "false", | ||
"fs": "ext4", | ||
"ssd": "0", | ||
"cpu": "4", | ||
"localSSD": "true", | ||
}), | ||
}, | ||
// Assert that release-blocker label exists when !nonReleaseBlocker | ||
// Also ensure that in the event of a failed cluster creation, | ||
// nil `vmOptions` and `clusterImpl` are not dereferenced | ||
{false, true, false, false, true, | ||
prefixAll(map[string]string{ | ||
"cloud": "gce", | ||
"ssd": "0", | ||
"cpu": "4", | ||
}), | ||
}, | ||
//Simulate failure loading TEAMS.yaml | ||
{true, false, true, false, false, nil}, | ||
} | ||
|
||
reg, _ := makeTestRegistry(spec.GCE, "", "", false) | ||
|
||
for _, c := range testCases { | ||
clusterSpec := reg.MakeClusterSpec(1) | ||
|
||
testSpec := ®istry.TestSpec{ | ||
Name: "githubPost", | ||
Owner: OwnerUnitTest, | ||
Cluster: clusterSpec, | ||
NonReleaseBlocker: c.nonReleaseBlocker, | ||
} | ||
|
||
ti := &testImpl{ | ||
spec: testSpec, | ||
l: nilLogger(), | ||
} | ||
|
||
testClusterImpl := &clusterImpl{spec: clusterSpec} | ||
vo := vm.DefaultCreateOpts() | ||
vmOpts := &vo | ||
|
||
if c.clusterCreationFailed { | ||
testClusterImpl = nil | ||
vmOpts = nil | ||
} else if !c.localSSD { | ||
// The default is true set in `vm.DefaultCreateOpts` | ||
vmOpts.SSDOpts.UseLocalSSD = false | ||
} | ||
|
||
teamLoadFn := validTeamsFn | ||
|
||
if c.loadTeamsFailed { | ||
teamLoadFn = invalidTeamsFn | ||
} | ||
|
||
github := &githubIssues{ | ||
vmCreateOpts: vmOpts, | ||
cluster: testClusterImpl, | ||
l: nilLogger(), | ||
teamLoader: teamLoadFn, | ||
} | ||
|
||
if c.loadTeamsFailed { | ||
// Assert that if TEAMS.yaml cannot be loaded then function panics. | ||
assert.Panics(t, func() { github.createPostRequest(ti, "message") }) | ||
} else { | ||
req := github.createPostRequest(ti, "message") | ||
|
||
if c.expectedParams != nil { | ||
require.Equal(t, c.expectedParams, req.ExtraParams) | ||
|
||
for k, v := range c.expectedParams { | ||
if req.ExtraParams[k] != v { | ||
require.Equal(t, v, req.ExtraParams[k]) | ||
} | ||
} | ||
} | ||
|
||
lastLabel := req.ExtraLabels[len(req.ExtraLabels)-1] | ||
if !c.nonReleaseBlocker { | ||
require.Equal(t, "release-blocker", lastLabel) | ||
} else { | ||
require.Equal(t, "O-roachtest", lastLabel) | ||
} | ||
} | ||
} | ||
} |
Oops, something went wrong.