[gold] Support multiple CRS systems
While not the most elegant solution, I found this to be the
easiest - bundling up the important CRS related objects and
passing around lists of these where applicable.
A follow-on CL will update all the lit-html pages to include
crs in the state.
There's an update to goldctl to provide crs when triaging
and for getting expectations.
One the remaining polymer pages are ported and the goldctl
change is rolled out, we can remove the code that defaults
the selection to the first review system (included for
backwards compatibility).
Change-Id: I3d503366091798b757e9fbeec4894b69dc0e4a50
Bug: skia:10532
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/306067
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
diff --git a/go/gerrit/gerrit.go b/go/gerrit/gerrit.go
index be5545d..28ded00 100644
--- a/go/gerrit/gerrit.go
+++ b/go/gerrit/gerrit.go
@@ -400,6 +400,7 @@
scanner := bufio.NewScanner(strings.NewReader(commitMsg))
for scanner.Scan() {
line := scanner.Text()
+ // Reminder, this regex has the review url (e.g. skia-review.googlesource.com) baked into it.
result := g.extractRegEx.FindStringSubmatch(line)
if len(result) == 2 {
ret, err := strconv.ParseInt(result[1], 10, 64)
diff --git a/gold-client/go/goldclient/goldclient.go b/gold-client/go/goldclient/goldclient.go
index bc4adf1..7fb7165 100644
--- a/gold-client/go/goldclient/goldclient.go
+++ b/gold-client/go/goldclient/goldclient.go
@@ -810,6 +810,7 @@
// Build TriageRequest struct and encode it into JSON.
triageRequest := &frontend.TriageRequest{
TestDigestStatus: map[types.TestName]map[types.Digest]expectations.Label{testName: {digest: expectations.Positive}},
+ CodeReviewSystem: c.resultState.SharedConfig.CodeReviewSystem,
ChangeListID: c.resultState.SharedConfig.ChangeListID,
ImageMatchingAlgorithm: algorithmName,
}
diff --git a/gold-client/go/goldclient/goldclient_test.go b/gold-client/go/goldclient/goldclient_test.go
index 027eac0..51f9d0f 100644
--- a/gold-client/go/goldclient/goldclient_test.go
+++ b/gold-client/go/goldclient/goldclient_test.go
@@ -3,6 +3,7 @@
import (
"bytes"
"context"
+ "encoding/json"
"image"
"image/png"
"io"
@@ -31,6 +32,7 @@
one_by_five "go.skia.org/infra/golden/go/testutils/data_one_by_five"
"go.skia.org/infra/golden/go/tiling"
"go.skia.org/infra/golden/go/types"
+ "go.skia.org/infra/golden/go/web/frontend"
)
// test data processing of the known hashes input.
@@ -48,7 +50,7 @@
httpClient.On("Get", "https://testing-gold.skia.org/json/hashes").Return(hashesResp, nil)
exp := httpResponse([]byte("{}"), "200 OK", http.StatusOK)
- httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=867").Return(exp, nil)
+ httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=867&crs=gerrit").Return(exp, nil)
goldClient, err := makeGoldClient(auth, false /*=passFail*/, false /*=uploadOnly*/, wd)
assert.NoError(t, err)
@@ -81,7 +83,7 @@
httpClient.On("Get", "https://testing-gold.skia.org/json/hashes").Return(hashesResp, nil)
exp := httpResponse([]byte(mockBaselineJSON), "200 OK", http.StatusOK)
- httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=867").Return(exp, nil)
+ httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=867&crs=gerrit").Return(exp, nil)
goldClient, err := makeGoldClient(auth, false /*=passFail*/, false /*=uploadOnly*/, wd)
assert.NoError(t, err)
@@ -162,7 +164,7 @@
httpClient.On("Get", "https://testing-gold.skia.org/json/hashes").Return(hashesResp, nil)
exp := httpResponse([]byte(mockBaselineJSON), "200 OK", http.StatusOK)
- httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=867").Return(exp, nil)
+ httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=867&crs=gerrit").Return(exp, nil)
// no uploader calls
@@ -331,7 +333,7 @@
httpClient.On("Get", "https://testing-gold.skia.org/json/hashes").Return(hashesResp, nil)
exp := httpResponse([]byte("{}"), "200 OK", http.StatusOK)
- httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=867").Return(exp, nil)
+ httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=867&crs=gerrit").Return(exp, nil)
expectedUploadPath := string("gs://skia-gold-testing/dm-images-v1/" + imgHash + ".png")
uploader.On("UploadBytes", testutils.AnyContext, imgData, testImgPath, expectedUploadPath).Return(nil)
@@ -372,7 +374,7 @@
httpClient.On("Get", "https://testing-gold.skia.org/json/hashes").Return(hashesResp, nil)
exp := httpResponse([]byte("{}"), "200 OK", http.StatusOK)
- httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=867").Return(exp, nil)
+ httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=867&crs=gerrit").Return(exp, nil)
// Notice the JSON is not uploaded if we are not in passfail mode - a client
// would need to call finalize first.
@@ -592,7 +594,7 @@
httpClient.On("Get", "https://testing-gold.skia.org/json/hashes").Return(hashesResp, nil)
exp := httpResponse([]byte("{}"), "200 OK", http.StatusOK)
- httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=867").Return(exp, nil)
+ httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=867&crs=gerrit").Return(exp, nil)
expectedUploadPath := string("gs://skia-gold-testing/dm-images-v1/" + imgHash + ".png")
uploader.On("UploadBytes", testutils.AnyContext, imgData, testImgPath, expectedUploadPath).Return(nil)
@@ -671,7 +673,7 @@
httpClient.On("Get", "https://testing-gold.skia.org/json/hashes").Return(hashesResp, nil)
exp := httpResponse([]byte(mockBaselineJSON), "200 OK", http.StatusOK)
- httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=867").Return(exp, nil)
+ httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=867&crs=gerrit").Return(exp, nil)
expectedJSONPath := "skia-gold-testing/trybot/dm-json-v1/2019/04/02/19/abcd1234/117/1554234843/dm-1554234843000000000.json"
checkResults := func(g *jsonio.GoldResults) bool {
@@ -748,7 +750,7 @@
httpClient.On("Get", "https://testing-gold.skia.org/json/hashes").Return(hashesResp, nil)
exp := httpResponse([]byte(mockBaselineJSON), "200 OK", http.StatusOK)
- httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=867").Return(exp, nil)
+ httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=867&crs=gerrit").Return(exp, nil)
expectedJSONPath := "skia-gold-testing/trybot/dm-json-v1/2019/04/02/19/abcd1234/117/1554234843/dm-1554234843000000000.json"
checkResults := func(g *jsonio.GoldResults) bool {
@@ -845,7 +847,7 @@
// Mock out getting the test baselines.
exp := httpResponse([]byte(mockBaselineJSON), "200 OK", http.StatusOK)
- httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=867").Return(exp, nil)
+ httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=867&crs=gerrit").Return(exp, nil)
// Mock out retrieving the latest positive image hash for ThisIsTheOnlyTest.
const latestPositiveDigestRpcUrl = "https://testing-gold.skia.org/json/latestpositivedigest/,another_notch=emeril,gpu=GPUTest,name=ThisIsTheOnlyTest,os=WinTest,source_type=gtest-pixeltests,"
@@ -856,8 +858,29 @@
downloader.On("DownloadImage", testutils.AnyContext, "https://testing-gold.skia.org", latestPositiveImageHash).Return(latestPositiveImageBytes, nil)
// Mock out RPC to automatically triage the new image as positive.
- body := bytes.NewReader([]byte(`{"testDigestStatus":{"ThisIsTheOnlyTest":{"` + newImageHash + `":"positive"}},"issue":"867","imageMatchingAlgorithm":"fuzzy"}`))
- httpClient.On("Post", "https://testing-gold.skia.org/json/triage", "application/json", body).Return(httpResponse([]byte{}, "200 OK", http.StatusOK), nil)
+ bodyMatcher := mock.MatchedBy(func(r io.Reader) bool {
+ b, err := ioutil.ReadAll(r)
+ assert.NoError(t, err)
+ if len(b) == 0 {
+ // This matcher can get called a second time during AssertExpectations. This check makes sure
+ // we don't erroniously fail.
+ return false
+ }
+ tr := frontend.TriageRequest{}
+ assert.NoError(t, json.Unmarshal(b, &tr))
+ assert.Equal(t, frontend.TriageRequest{
+ TestDigestStatus: map[types.TestName]map[types.Digest]expectations.Label{
+ "ThisIsTheOnlyTest": {
+ newImageHash: expectations.Positive,
+ },
+ },
+ ChangeListID: "867",
+ CodeReviewSystem: "gerrit",
+ ImageMatchingAlgorithm: "fuzzy",
+ }, tr)
+ return true
+ })
+ httpClient.On("Post", "https://testing-gold.skia.org/json/triage", "application/json", bodyMatcher).Return(httpResponse([]byte{}, "200 OK", http.StatusOK), nil)
// Mock out uploading the JSON file with the test results to Gold.
expectedJSONPath := "skia-gold-testing/trybot/dm-json-v1/2019/04/02/19/abcd1234/117/1554234843/dm-1554234843000000000.json"
@@ -939,7 +962,7 @@
httpClient.On("Get", "https://testing-gold.skia.org/json/hashes").Return(hashesResp, nil)
exp := httpResponse([]byte(mockBaselineJSON), "200 OK", http.StatusOK)
- httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=867").Return(exp, nil)
+ httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=867&crs=gerrit").Return(exp, nil)
// No upload expected because the bytes were already seen in json/hashes.
@@ -994,7 +1017,7 @@
httpClient.On("Get", "https://testing-gold.skia.org/json/hashes").Return(hashesResp, nil)
exp := httpResponse([]byte(mockBaselineJSON), "200 OK", http.StatusOK)
- httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=867").Return(exp, nil)
+ httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=867&crs=gerrit").Return(exp, nil)
// No upload expected because the bytes were already seen in json/hashes.
@@ -1152,9 +1175,10 @@
imgData := []byte("some bytes")
// These are defined in mockBaselineJSON
- imgHash := types.Digest("beef00d3a1527db19619ec12a4e0df68")
- testName := types.TestName("ThisIsTheOnlyTest")
- changeListID := "abc"
+ const imgHash = types.Digest("beef00d3a1527db19619ec12a4e0df68")
+ const testName = types.TestName("ThisIsTheOnlyTest")
+ const githubCRS = "github"
+ const changeListID = "abc"
auth, httpClient, _, _ := makeMocks()
defer httpClient.AssertExpectations(t)
@@ -1163,7 +1187,7 @@
httpClient.On("Get", "https://testing-gold.skia.org/json/hashes").Return(hashesResp, nil)
exp := httpResponse([]byte(mockBaselineJSON), "200 OK", http.StatusOK)
- httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=abc").Return(exp, nil)
+ httpClient.On("Get", "https://testing-gold.skia.org/json/expectations?issue=abc&crs=github").Return(exp, nil)
config := GoldClientConfig{
WorkDir: wd,
@@ -1173,8 +1197,9 @@
assert.NoError(t, err)
gr := jsonio.GoldResults{
- ChangeListID: changeListID,
- GitHash: "HEAD",
+ CodeReviewSystem: githubCRS,
+ ChangeListID: changeListID,
+ GitHash: "HEAD",
}
err = goldClient.SetSharedConfig(gr, true)
assert.NoError(t, err)
@@ -2126,8 +2151,27 @@
url := "https://testing-gold.skia.org/json/triage"
contentType := "application/json"
- body := bytes.NewReader([]byte(`{"testDigestStatus":{"MyTest":{"deadbeefcafefe771d61bf0ed3d84bc2":"positive"}},"issue":"","imageMatchingAlgorithm":"fuzzy"}`))
- httpClient.On("Post", url, contentType, body).Return(httpResponse([]byte{}, "200 OK", http.StatusOK), nil)
+ bodyMatcher := mock.MatchedBy(func(r io.Reader) bool {
+ b, err := ioutil.ReadAll(r)
+ assert.NoError(t, err)
+ if len(b) == 0 {
+ // This matcher can get called a second time during AssertExpectations. This check makes sure
+ // we don't erroniously fail.
+ return false
+ }
+ tr := frontend.TriageRequest{}
+ assert.NoError(t, json.Unmarshal(b, &tr))
+ assert.Equal(t, frontend.TriageRequest{
+ TestDigestStatus: map[types.TestName]map[types.Digest]expectations.Label{
+ "MyTest": {
+ "deadbeefcafefe771d61bf0ed3d84bc2": expectations.Positive,
+ },
+ },
+ ImageMatchingAlgorithm: "fuzzy",
+ }, tr)
+ return true
+ })
+ httpClient.On("Post", url, contentType, bodyMatcher).Return(httpResponse([]byte{}, "200 OK", http.StatusOK), nil)
err = goldClient.TriageAsPositive("MyTest", "deadbeefcafefe771d61bf0ed3d84bc2", "fuzzy")
assert.NoError(t, err)
@@ -2144,7 +2188,8 @@
j := resultState{
GoldURL: "https://testing-gold.skia.org",
SharedConfig: &jsonio.GoldResults{
- ChangeListID: "123456",
+ CodeReviewSystem: "gerrit",
+ ChangeListID: "123456",
},
}
jsonToWrite := testutils.MarshalJSON(t, &j)
@@ -2158,8 +2203,29 @@
url := "https://testing-gold.skia.org/json/triage"
contentType := "application/json"
- body := bytes.NewReader([]byte(`{"testDigestStatus":{"MyTest":{"deadbeefcafefe771d61bf0ed3d84bc2":"positive"}},"issue":"123456","imageMatchingAlgorithm":"fuzzy"}`))
- httpClient.On("Post", url, contentType, body).Return(httpResponse([]byte{}, "200 OK", http.StatusOK), nil)
+ bodyMatcher := mock.MatchedBy(func(r io.Reader) bool {
+ b, err := ioutil.ReadAll(r)
+ assert.NoError(t, err)
+ if len(b) == 0 {
+ // This matcher can get called a second time during AssertExpectations. This check makes sure
+ // we don't erroniously fail.
+ return false
+ }
+ tr := frontend.TriageRequest{}
+ assert.NoError(t, json.Unmarshal(b, &tr))
+ assert.Equal(t, frontend.TriageRequest{
+ TestDigestStatus: map[types.TestName]map[types.Digest]expectations.Label{
+ "MyTest": {
+ "deadbeefcafefe771d61bf0ed3d84bc2": expectations.Positive,
+ },
+ },
+ CodeReviewSystem: "gerrit",
+ ChangeListID: "123456",
+ ImageMatchingAlgorithm: "fuzzy",
+ }, tr)
+ return true
+ })
+ httpClient.On("Post", url, contentType, bodyMatcher).Return(httpResponse([]byte{}, "200 OK", http.StatusOK), nil)
err = goldClient.TriageAsPositive("MyTest", "deadbeefcafefe771d61bf0ed3d84bc2", "fuzzy")
assert.NoError(t, err)
@@ -2174,10 +2240,8 @@
// Pretend "goldctl imgtest init" was called.
j := resultState{
- GoldURL: "https://testing-gold.skia.org",
- SharedConfig: &jsonio.GoldResults{
- ChangeListID: "123456",
- },
+ GoldURL: "https://testing-gold.skia.org",
+ SharedConfig: &jsonio.GoldResults{},
}
jsonToWrite := testutils.MarshalJSON(t, &j)
testutils.WriteFile(t, filepath.Join(wd, stateFile), jsonToWrite)
@@ -2190,8 +2254,7 @@
url := "https://testing-gold.skia.org/json/triage"
contentType := "application/json"
- body := bytes.NewReader([]byte(`{"testDigestStatus":{"MyTest":{"deadbeefcafefe771d61bf0ed3d84bc2":"positive"}},"issue":"123456","imageMatchingAlgorithm":"fuzzy"}`))
- httpClient.On("Post", url, contentType, body).Return(httpResponse([]byte{}, "500 Internal Server Error", http.StatusInternalServerError), nil)
+ httpClient.On("Post", url, contentType, mock.Anything).Return(httpResponse([]byte{}, "500 Internal Server Error", http.StatusInternalServerError), nil)
err = goldClient.TriageAsPositive("MyTest", "deadbeefcafefe771d61bf0ed3d84bc2", "fuzzy")
assert.Error(t, err)
diff --git a/gold-client/go/goldclient/resultstate.go b/gold-client/go/goldclient/resultstate.go
index 30841b1..c8fc4c5 100644
--- a/gold-client/go/goldclient/resultstate.go
+++ b/gold-client/go/goldclient/resultstate.go
@@ -123,7 +123,7 @@
func (r *resultState) loadExpectations(httpClient HTTPClient) error {
urlPath := shared.ExpectationsRoute
if r.SharedConfig != nil && r.SharedConfig.ChangeListID != "" {
- urlPath = fmt.Sprintf("%s?issue=%s", urlPath, url.QueryEscape(r.SharedConfig.ChangeListID))
+ urlPath = fmt.Sprintf("%s?issue=%s&crs=%s", urlPath, url.QueryEscape(r.SharedConfig.ChangeListID), url.QueryEscape(r.SharedConfig.CodeReviewSystem))
}
u := fmt.Sprintf("%s/%s", r.GoldURL, strings.TrimLeft(urlPath, "/"))
diff --git a/golden/cmd/baseline_server/main.go b/golden/cmd/baseline_server/main.go
index 0027ba5..56558f8 100644
--- a/golden/cmd/baseline_server/main.go
+++ b/golden/cmd/baseline_server/main.go
@@ -12,6 +12,7 @@
"path/filepath"
"github.com/gorilla/mux"
+ "go.skia.org/infra/golden/go/clstore"
gstorage "google.golang.org/api/storage/v1"
"go.skia.org/infra/go/auth"
@@ -20,7 +21,6 @@
"go.skia.org/infra/go/httputils"
"go.skia.org/infra/go/sklog"
"go.skia.org/infra/golden/go/baseline/simple_baseliner"
- "go.skia.org/infra/golden/go/clstore/fs_clstore"
"go.skia.org/infra/golden/go/config"
"go.skia.org/infra/golden/go/expectations/fs_expectationstore"
"go.skia.org/infra/golden/go/shared"
@@ -102,15 +102,18 @@
sklog.Fatalf("Unable to create GCSClient: %s", err)
}
- // Baseline doesn't need to access this, just needs a way to indicate which CRS we are on.
- emptyCLStore := fs_clstore.New(nil, bsc.PrimaryCRS)
+ // Baselines just need a list of valid CRS; we can leave all other fields blank.
+ var reviewSystems []clstore.ReviewSystem
+ for _, cfg := range bsc.CodeReviewSystems {
+ reviewSystems = append(reviewSystems, clstore.ReviewSystem{ID: cfg.ID})
+ }
// We only need to fill in the HandlersConfig struct with the following subset, since the baseline
// server only supplies a subset of the functionality.
handlers, err := web.NewHandlers(web.HandlersConfig{
- GCSClient: gsClient,
- Baseliner: baseliner,
- ChangeListStore: emptyCLStore,
+ Baseliner: baseliner,
+ GCSClient: gsClient,
+ ReviewSystems: reviewSystems,
}, web.BaselineSubset)
if err != nil {
sklog.Fatalf("Failed to initialize web handlers: %s", err)
diff --git a/golden/cmd/skiacorrectness/main.go b/golden/cmd/skiacorrectness/main.go
index b2fc937..0b67f01 100644
--- a/golden/cmd/skiacorrectness/main.go
+++ b/golden/cmd/skiacorrectness/main.go
@@ -29,11 +29,13 @@
"go.skia.org/infra/go/gitstore/bt_gitstore"
"go.skia.org/infra/go/httputils"
"go.skia.org/infra/go/login"
+ "go.skia.org/infra/go/skerr"
"go.skia.org/infra/go/sklog"
"go.skia.org/infra/go/timer"
"go.skia.org/infra/go/util"
"go.skia.org/infra/go/vcsinfo/bt_vcs"
"go.skia.org/infra/golden/go/baseline/simple_baseliner"
+ "go.skia.org/infra/golden/go/clstore"
"go.skia.org/infra/golden/go/clstore/fs_clstore"
"go.skia.org/infra/golden/go/code_review"
"go.skia.org/infra/golden/go/code_review/commenter"
@@ -85,10 +87,7 @@
// Client secret file for OAuth2 authentication.
ClientSecretFile string `json:"client_secret_file"`
- // A URL with %s where a CL ID should be placed to complete it.
- CodeReviewSystemURLTemplate string `json:"crs_url_template"`
-
- // If true, Gold will only log comments, it won't actually comment on the CRS.
+ // If true, Gold will only log comments, it won't actually comment on the CRSes.
DisableCLComments bool `json:"disable_cl_comments"`
// The grpc port of the diff server.
@@ -111,15 +110,6 @@
// Configuration settings that will get passed to the frontend (see modules/settings.js)
FrontendConfig frontendConfig `json:"frontend"`
- // URL of the Gerrit instance (if any) where we retrieve CL metadata.
- GerritURL string `json:"gerrit_url" optional:"true"`
-
- // Filepath to file containing GitHub token (if this instance needs to talk to GitHub).
- GitHubCredPath string `json:"github_cred_path" optional:"true"`
-
- // User and repo of GitHub project to connect to (if any), e.g. google/skia
- GitHubRepo string `json:"github_repo" optional:"true"`
-
// If this instance is simply a mirror of another instance's data.
IsPublicView bool `json:"is_public_view"`
@@ -349,38 +339,15 @@
sklog.Fatalf("Failed to start monitoring for expired ignore rules: %s", err)
}
- cls := fs_clstore.New(fsClient, fsc.PrimaryCRS)
tjs := fs_tjstore.New(fsClient)
-
- var crs code_review.Client
- if fsc.PrimaryCRS == "gerrit" {
- if fsc.GerritURL == "" {
- sklog.Fatalf("You must specify gerrit_url")
- }
- gerritClient, err := gerrit.NewGerrit(fsc.GerritURL, client)
- if err != nil {
- sklog.Fatalf("Could not create gerrit client for %s", fsc.GerritURL)
- }
- crs = gerrit_crs.New(gerritClient)
- } else if fsc.PrimaryCRS == "github" {
- if fsc.GitHubRepo == "" || fsc.GitHubCredPath == "" {
- sklog.Fatalf("You must specify github_repo and github_cred_path")
- }
- gBody, err := ioutil.ReadFile(fsc.GitHubCredPath)
- if err != nil {
- sklog.Fatalf("Couldn't find githubToken in %s: %s", fsc.GitHubCredPath, err)
- }
- gToken := strings.TrimSpace(string(gBody))
- githubTS := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: gToken})
- c := httputils.DefaultClientConfig().With2xxOnly().WithTokenSource(githubTS).Client()
- crs = github_crs.New(c, fsc.GitHubRepo)
- } else {
- sklog.Fatalf("CRS %s not supported.", fsc.PrimaryCRS)
+ reviewSystems, err := initializeReviewSystems(fsc.CodeReviewSystems, fsClient, client)
+ if err != nil {
+ sklog.Fatalf("Could not initialize CRS: %s", err)
}
var clUpdater code_review.ChangeListLandedUpdater
if isAuthoritative && !fsc.DisableCLTracking {
- clUpdater = updater.New(crs, expStore, cls)
+ clUpdater = updater.New(expStore, reviewSystems)
}
ctc := tilesource.CachedTileSourceConfig{
@@ -401,14 +368,14 @@
}
ic := indexer.IndexerConfig{
- DiffStore: diffStore,
ChangeListener: expChangeHandler,
+ DiffStore: diffStore,
ExpectationsStore: expStore,
GCSClient: gsClient,
+ ReviewSystems: reviewSystems,
TileSource: tileSource,
- Warmer: warmer.New(),
TryJobStore: tjs,
- CLStore: cls,
+ Warmer: warmer.New(),
}
// Rebuild the index every few minutes.
@@ -420,16 +387,18 @@
sklog.Infof("Indexer created.")
// TODO(kjlubick) include non-nil comment.Store when it is implemented.
- searchAPI := search.New(diffStore, expStore, expChangeHandler, ixr, cls, tjs, nil, publiclyViewableParams, fsc.FlakyTraceThreshold)
+ searchAPI := search.New(diffStore, expStore, expChangeHandler, ixr, reviewSystems, tjs, nil, publiclyViewableParams, fsc.FlakyTraceThreshold)
sklog.Infof("Search API created")
if isAuthoritative && !fsc.DisableCLTracking {
- clCommenter, err := commenter.New(crs, cls, searchAPI, fsc.CLCommentTemplate, fsc.SiteURL, fsc.PublicSiteURL, fsc.DisableCLComments)
- if err != nil {
- sklog.Fatalf("Could not initialize commenter: %s", err)
+ for _, rs := range reviewSystems {
+ clCommenter, err := commenter.New(rs, searchAPI, fsc.CLCommentTemplate, fsc.SiteURL, fsc.PublicSiteURL, fsc.DisableCLComments)
+ if err != nil {
+ sklog.Fatalf("Could not initialize commenter: %s", err)
+ }
+ startCommenter(ctx, clCommenter)
}
- startCommenter(ctx, clCommenter)
}
swc := status.StatusWatcherConfig{
@@ -462,19 +431,18 @@
}
handlers, err := web.NewHandlers(web.HandlersConfig{
- Baseliner: baseliner,
- ChangeListStore: cls,
- CodeReviewURLTemplate: fsc.CodeReviewSystemURLTemplate,
- DiffStore: diffStore,
- ExpectationsStore: expStore,
- GCSClient: gsClient,
- IgnoreStore: ignoreStore,
- Indexer: ixr,
- SearchAPI: searchAPI,
- StatusWatcher: statusWatcher,
- TileSource: tileSource,
- TryJobStore: tjs,
- VCS: vcs,
+ Baseliner: baseliner,
+ DiffStore: diffStore,
+ ExpectationsStore: expStore,
+ GCSClient: gsClient,
+ IgnoreStore: ignoreStore,
+ Indexer: ixr,
+ ReviewSystems: reviewSystems,
+ SearchAPI: searchAPI,
+ StatusWatcher: statusWatcher,
+ TileSource: tileSource,
+ TryJobStore: tjs,
+ VCS: vcs,
}, web.FullFrontEnd)
if err != nil {
sklog.Fatalf("Failed to initialize web handlers: %s", err)
@@ -558,7 +526,6 @@
loadTemplates()
fsc.FrontendConfig.BaseRepoURL = fsc.GitRepoURL
- fsc.FrontendConfig.CodeReviewTemplate = fsc.CodeReviewSystemURLTemplate
fsc.FrontendConfig.IsPublic = fsc.IsPublicView
templateHandler := func(name string) http.HandlerFunc {
@@ -621,12 +588,51 @@
sklog.Fatal(http.ListenAndServe(fsc.Port, rootRouter))
}
+func initializeReviewSystems(configs []config.CodeReviewSystem, fc *firestore.Client, hc *http.Client) ([]clstore.ReviewSystem, error) {
+ rs := make([]clstore.ReviewSystem, 0, len(configs))
+ for _, cfg := range configs {
+ var crs code_review.Client
+ if cfg.Flavor == "gerrit" {
+ if cfg.GerritURL == "" {
+ return nil, skerr.Fmt("You must specify gerrit_url")
+ }
+ gerritClient, err := gerrit.NewGerrit(cfg.GerritURL, hc)
+ if err != nil {
+ return nil, skerr.Fmt("Could not create gerrit client for %s", cfg.GerritURL)
+ }
+ crs = gerrit_crs.New(gerritClient)
+ } else if cfg.Flavor == "github" {
+ if cfg.GitHubRepo == "" || cfg.GitHubCredPath == "" {
+ return nil, skerr.Fmt("You must specify github_repo and github_cred_path")
+ }
+ gBody, err := ioutil.ReadFile(cfg.GitHubCredPath)
+ if err != nil {
+ return nil, skerr.Fmt("Couldn't find githubToken in %s: %s", cfg.GitHubCredPath, err)
+ }
+ gToken := strings.TrimSpace(string(gBody))
+ githubTS := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: gToken})
+ c := httputils.DefaultClientConfig().With2xxOnly().WithTokenSource(githubTS).Client()
+ crs = github_crs.New(c, cfg.GitHubRepo)
+ } else {
+ return nil, skerr.Fmt("CRS flavor %s not supported.", cfg.Flavor)
+ }
+
+ rs = append(rs, clstore.ReviewSystem{
+ ID: cfg.ID,
+ Client: crs,
+ Store: fs_clstore.New(fc, cfg.ID),
+ URLTemplate: cfg.URLTemplate,
+ })
+ }
+ sklog.Infof("Review systems %#v", rs)
+ return rs, nil
+}
+
type frontendConfig struct {
- BaseRepoURL string `json:"baseRepoURL"`
- CodeReviewTemplate string `json:"crsTemplate"`
- DefaultCorpus string `json:"defaultCorpus"`
- Title string `json:"title"`
- IsPublic bool `json:"isPublic"`
+ BaseRepoURL string `json:"baseRepoURL"`
+ DefaultCorpus string `json:"defaultCorpus"`
+ Title string `json:"title"`
+ IsPublic bool `json:"isPublic"`
}
// startCommenter begins the background process that comments on CLs.
diff --git a/golden/go/clstore/clstore.go b/golden/go/clstore/clstore.go
index 4bef575..76b551d 100644
--- a/golden/go/clstore/clstore.go
+++ b/golden/go/clstore/clstore.go
@@ -17,6 +17,8 @@
// the purpose of this interface is not to store every CL.
// A single Store interface should only be responsible for one "system", i.e.
// Gerrit or GitHub.
+// TODO(kjlubick) Just like the tryjobstore holds onto all tryjob results (from all CIS), this
+// should hold onto all CLs from all CRS.
type Store interface {
// GetChangeList returns the ChangeList corresponding to the given id.
// Returns NotFound if it doesn't exist.
@@ -46,9 +48,6 @@
// PutPatchSet stores the given PatchSet, overwriting any values for
// that PatchSet if they already existed.
PutPatchSet(ctx context.Context, ps code_review.PatchSet) error
-
- // System returns the underlying system (e.g. "gerrit")
- System() string
}
var ErrNotFound = errors.New("not found")
@@ -64,3 +63,11 @@
// CountMany indicates it is computationally expensive to determine exactly how many
// items there are.
var CountMany = math.MaxInt32
+
+// ReviewSystem combines the data needed to interface with a single CRS.
+type ReviewSystem struct {
+ ID string // e.g. "gerrit", "gerrit-internal"
+ Client code_review.Client
+ Store Store
+ URLTemplate string
+}
diff --git a/golden/go/clstore/fs_clstore/fs_clstore.go b/golden/go/clstore/fs_clstore/fs_clstore.go
index 56aadab..379381d 100644
--- a/golden/go/clstore/fs_clstore/fs_clstore.go
+++ b/golden/go/clstore/fs_clstore/fs_clstore.go
@@ -34,15 +34,16 @@
// StoreImpl is the Firestore based implementation of clstore.
type StoreImpl struct {
- client *ifirestore.Client
- crsName string
+ client *ifirestore.Client
+ crsID string
}
-// New returns a new StoreImpl
-func New(client *ifirestore.Client, crsName string) *StoreImpl {
+// New returns a new StoreImpl. The crsID should be distinct enough to separate an internal CRS
+// from an external one, if needed (e.g. "gerrit" vs "gerrit-internal")
+func New(client *ifirestore.Client, crsID string) *StoreImpl {
return &StoreImpl{
- client: client,
- crsName: crsName,
+ client: client,
+ crsID: crsID,
}
}
@@ -97,7 +98,7 @@
cle := changeListEntry{}
if err := doc.DataTo(&cle); err != nil {
id := doc.Ref.ID
- return code_review.ChangeList{}, skerr.Wrapf(err, "corrupt data in Firestore, could not unmarshal %s changelist with id %s", s.crsName, id)
+ return code_review.ChangeList{}, skerr.Wrapf(err, "corrupt data in Firestore, could not unmarshal %s changelist with id %s", s.crsID, id)
}
cl := code_review.ChangeList{
SystemID: cle.SystemID,
@@ -113,7 +114,7 @@
// changeListFirestoreID returns the id for a given CL in a given CRS - this allows us to
// look up a document by id w/o having to perform a query.
func (s *StoreImpl) changeListFirestoreID(clID string) string {
- return clID + "_" + s.crsName
+ return clID + "_" + s.crsID
}
// GetChangeLists implements the clstore.Store interface.
@@ -188,7 +189,7 @@
pse := patchSetEntry{}
if err := doc.DataTo(&pse); err != nil {
id := doc.Ref.ID
- return code_review.PatchSet{}, skerr.Wrapf(err, "corrupt data in Firestore, could not unmarshal %s patchset with id %s", s.crsName, id)
+ return code_review.PatchSet{}, skerr.Wrapf(err, "corrupt data in Firestore, could not unmarshal %s patchset with id %s", s.crsID, id)
}
return pse.toPatchSet(), nil
}
@@ -261,7 +262,7 @@
cd := s.client.Collection(changelistCollection).Doc(fID)
record := changeListEntry{
SystemID: cl.SystemID,
- System: s.crsName,
+ System: s.crsID,
Owner: cl.Owner,
Status: cl.Status,
Subject: cl.Subject,
@@ -282,7 +283,7 @@
Collection(patchsetCollection).Doc(ps.SystemID)
record := patchSetEntry{
SystemID: ps.SystemID,
- System: s.crsName,
+ System: s.crsID,
ChangeListID: ps.ChangeListID,
Order: ps.Order,
GitHash: ps.GitHash,
@@ -296,10 +297,5 @@
return nil
}
-// System implements the clstore.Store interface.
-func (s *StoreImpl) System() string {
- return s.crsName
-}
-
// Make sure StoreImpl fulfills the clstore.Store interface.
var _ clstore.Store = (*StoreImpl)(nil)
diff --git a/golden/go/clstore/mocks/Store.go b/golden/go/clstore/mocks/Store.go
index 21ed0c1..23fff9c 100644
--- a/golden/go/clstore/mocks/Store.go
+++ b/golden/go/clstore/mocks/Store.go
@@ -159,17 +159,3 @@
return r0
}
-
-// System provides a mock function with given fields:
-func (_m *Store) System() string {
- ret := _m.Called()
-
- var r0 string
- if rf, ok := ret.Get(0).(func() string); ok {
- r0 = rf()
- } else {
- r0 = ret.Get(0).(string)
- }
-
- return r0
-}
diff --git a/golden/go/code_review/commenter/commenter.go b/golden/go/code_review/commenter/commenter.go
index e0e203d..69972af 100644
--- a/golden/go/code_review/commenter/commenter.go
+++ b/golden/go/code_review/commenter/commenter.go
@@ -27,8 +27,7 @@
)
type Impl struct {
- crs code_review.Client
- store clstore.Store
+ system clstore.ReviewSystem
instanceURL string
publicURL string
logCommentsOnly bool
@@ -40,14 +39,13 @@
now func() time.Time
}
-func New(c code_review.Client, s clstore.Store, search search.SearchAPI, messageTemplate, instanceURL, publicURL string, logCommentsOnly bool) (*Impl, error) {
+func New(system clstore.ReviewSystem, search search.SearchAPI, messageTemplate, instanceURL, publicURL string, logCommentsOnly bool) (*Impl, error) {
templ, err := template.New("message").Parse(messageTemplate)
if err != nil && messageTemplate != "" {
return nil, skerr.Wrapf(err, "Message template %q", messageTemplate)
}
return &Impl{
- crs: c,
- store: s,
+ system: system,
instanceURL: instanceURL,
publicURL: publicURL,
logCommentsOnly: logCommentsOnly,
@@ -69,7 +67,7 @@
// to look at CLs that were Updated "recently". We make the range of time that we search
// much wider than we need to account for either glitches in ingestion or outages of the CRS.
recent := i.now().Add(-timePeriodOfCLsToCheck)
- xcl, _, err := i.store.GetChangeLists(ctx, clstore.SearchOptions{
+ xcl, _, err := i.system.Store.GetChangeLists(ctx, clstore.SearchOptions{
StartIdx: 0,
Limit: pageSize,
OpenCLsOnly: true,
@@ -121,7 +119,7 @@
// Page to the next ones using len(stillOpen) because the next iteration of this query
// won't count the ones we just marked as Closed/Abandoned when computing the offset.
- xcl, _, err = i.store.GetChangeLists(ctx, clstore.SearchOptions{
+ xcl, _, err = i.system.Store.GetChangeLists(ctx, clstore.SearchOptions{
StartIdx: len(stillOpen),
Limit: pageSize,
OpenCLsOnly: true,
@@ -135,7 +133,7 @@
sklog.Infof("There were originally %d recent open CLs; after checking with CRS there are %d still open", total, len(stillOpen))
for _, cl := range stillOpen {
- xps, err := i.store.GetPatchSets(ctx, cl.SystemID)
+ xps, err := i.system.Store.GetPatchSets(ctx, cl.SystemID)
if err != nil {
return skerr.Wrapf(err, "looking for patchsets on open CL %s", cl.SystemID)
}
@@ -157,7 +155,7 @@
}
mostRecentPS.CommentedOnCL = true
}
- if err := i.store.PutPatchSet(ctx, mostRecentPS); err != nil {
+ if err := i.system.Store.PutPatchSet(ctx, mostRecentPS); err != nil {
return skerr.Wrapf(err, "updating PS %#v", mostRecentPS)
}
}
@@ -169,9 +167,8 @@
// maybeCommentOn either comments on the given CL/PS that there are untriaged digests on it or
// logs if this commenter is configured to not actually comment.
func (i *Impl) maybeCommentOn(ctx context.Context, cl code_review.ChangeList, ps code_review.PatchSet, untriagedDigests int) error {
- crs := i.crs.System()
msg, err := i.untriagedMessage(commentTemplateContext{
- CRS: crs,
+ CRS: i.system.ID,
ChangeListID: cl.SystemID,
PatchSetOrder: ps.Order,
NumUntriaged: untriagedDigests,
@@ -183,12 +180,12 @@
sklog.Infof("Should comment on CL %s with message %s", cl.SystemID, msg)
return nil
}
- if err := i.crs.CommentOn(ctx, cl.SystemID, msg); err != nil {
+ if err := i.system.Client.CommentOn(ctx, cl.SystemID, msg); err != nil {
if err == code_review.ErrNotFound {
- sklog.Warningf("Cannot comment on %s CL %s because it does not exist", i.crs.System(), cl.SystemID)
+ sklog.Warningf("Cannot comment on %s CL %s because it does not exist", i.system.ID, cl.SystemID)
return nil
}
- return skerr.Wrapf(err, "commenting on %s CL %s", i.crs.System(), cl.SystemID)
+ return skerr.Wrapf(err, "commenting on %s CL %s", i.system.ID, cl.SystemID)
}
return nil
}
@@ -218,13 +215,13 @@
// returns true. If it is Abandoned, it stores the updated CL in the store and returns false.
// If the CL is Landed, it returns false and *does not update anything* in the store.
func (i *Impl) updateCLInStoreIfAbandoned(ctx context.Context, cl code_review.ChangeList) (bool, error) {
- up, err := i.crs.GetChangeList(ctx, cl.SystemID)
+ up, err := i.system.Client.GetChangeList(ctx, cl.SystemID)
if err == code_review.ErrNotFound {
sklog.Debugf("CL %s might have been deleted", cl.SystemID)
return false, nil
}
if err != nil {
- return false, skerr.Wrapf(err, "querying crs %s for updated CL %s", i.crs.System(), cl.SystemID)
+ return false, skerr.Wrapf(err, "querying crs %s for updated CL %s", i.system.ID, cl.SystemID)
}
if up.Status == code_review.Open {
return true, nil
@@ -239,7 +236,7 @@
// remember it is abandoned in the future. This also catches things like the cl Subject
// changing since it was opened.
up.Updated = i.now()
- if err := i.store.PutChangeList(ctx, up); err != nil {
+ if err := i.system.Store.PutChangeList(ctx, up); err != nil {
return false, skerr.Wrapf(err, "storing CL %s", up.SystemID)
}
return false, nil
@@ -251,7 +248,7 @@
func (i *Impl) searchIndexForNewUntriagedDigests(ctx context.Context, clID, psID string) (int, time.Time) {
digestList, err := i.search.UntriagedUnignoredTryJobExclusiveDigests(ctx, tjstore.CombinedPSID{
CL: clID,
- CRS: i.crs.System(),
+ CRS: i.system.ID,
PS: psID,
})
if err != nil {
diff --git a/golden/go/code_review/commenter/commenter_test.go b/golden/go/code_review/commenter/commenter_test.go
index 5bf254d..64936b8 100644
--- a/golden/go/code_review/commenter/commenter_test.go
+++ b/golden/go/code_review/commenter/commenter_test.go
@@ -152,7 +152,6 @@
mcs.On("GetChangeLists", testutils.AnyContext, optionsMatcher).Return(makeChangeLists(5), 5, nil)
mcr.On("GetChangeList", testutils.AnyContext, mock.Anything).Return(code_review.ChangeList{}, errors.New("GitHub down"))
- mcr.On("System").Return("github")
c := newTestCommenter(t, mcr, mcs, nil)
err := c.CommentOnChangeListsWithUntriagedDigests(context.Background())
@@ -240,7 +239,6 @@
assert.Contains(t, msg, publicInstanceURL)
return nil
}, nil)
- mcr.On("System").Return("github")
// Pretend all CLs queried have 2 untriaged digests.
msa.On("UntriagedUnignoredTryJobExclusiveDigests", testutils.AnyContext, mock.Anything).Return(&frontend.UntriagedDigestList{
@@ -287,7 +285,6 @@
assert.NoError(t, err)
return xcl[i]
}, nil)
- mcr.On("System").Return("github")
// Pretend all CLs queried have 2 untriaged digests.
msa.On("UntriagedUnignoredTryJobExclusiveDigests", testutils.AnyContext, mock.Anything).Return(&frontend.UntriagedDigestList{
@@ -329,7 +326,6 @@
assert.NoError(t, err)
return xcl[i]
}, nil)
- mcr.On("System").Return("github")
// Simulate an error working with the
msa.On("UntriagedUnignoredTryJobExclusiveDigests", testutils.AnyContext, mock.Anything).Return(nil, errors.New("boom"))
@@ -370,7 +366,6 @@
assert.NoError(t, err)
return xcl[i]
}, nil)
- mcr.On("System").Return("github")
// Pretend all CLs queried have 2 untriaged digests.
msa.On("UntriagedUnignoredTryJobExclusiveDigests", testutils.AnyContext, mock.Anything).Return(&frontend.UntriagedDigestList{
@@ -456,7 +451,6 @@
return xcl[i]
}, nil)
mcr.On("CommentOn", testutils.AnyContext, mock.Anything, mock.Anything).Return(errors.New("internet down"))
- mcr.On("System").Return("gerritHub")
// Pretend all CLs queried have 2 untriaged digests.
msa.On("UntriagedUnignoredTryJobExclusiveDigests", testutils.AnyContext, mock.Anything).Return(&frontend.UntriagedDigestList{
@@ -490,7 +484,6 @@
return xcl[i]
}, nil)
mcr.On("CommentOn", testutils.AnyContext, mock.Anything, mock.Anything).Return(code_review.ErrNotFound)
- mcr.On("System").Return("gerritHub")
// We should see two PatchSets with their CommentedOnCL bit set written back to Firestore.
// Even though we are logging the comments, we want to update Firestore that we "commented".
@@ -559,7 +552,12 @@
}
func newTestCommenter(t *testing.T, mcr *mock_codereview.Client, mcs *mock_clstore.Store, msa *mock_search.SearchAPI) *Impl {
- c, err := New(mcr, mcs, msa, basicTemplate, instanceURL, publicInstanceURL, false)
+ c, err := New(clstore.ReviewSystem{
+ ID: "github",
+ Client: mcr,
+ Store: mcs,
+ // URLTemplate not needed here
+ }, msa, basicTemplate, instanceURL, publicInstanceURL, false)
require.NoError(t, err)
c.now = func() time.Time {
return fakeNow
diff --git a/golden/go/code_review/mocks/Client.go b/golden/go/code_review/mocks/Client.go
index acd4300..272ee88 100644
--- a/golden/go/code_review/mocks/Client.go
+++ b/golden/go/code_review/mocks/Client.go
@@ -95,17 +95,3 @@
return r0, r1
}
-
-// System provides a mock function with given fields:
-func (_m *Client) System() string {
- ret := _m.Called()
-
- var r0 string
- if rf, ok := ret.Get(0).(func() string); ok {
- r0 = rf()
- } else {
- r0 = ret.Get(0).(string)
- }
-
- return r0
-}
diff --git a/golden/go/code_review/types.go b/golden/go/code_review/types.go
index 8273536..c9963a9 100644
--- a/golden/go/code_review/types.go
+++ b/golden/go/code_review/types.go
@@ -27,9 +27,6 @@
// CommentOn creates a comment on the CRS for the given CL with the given message.
CommentOn(ctx context.Context, clID, message string) error
-
- // System returns the underlying system (e.g. "gerrit")
- System() string
}
// The ChangeListLandedUpdater interface is an abstraction around the code that tracks ChangeLists
diff --git a/golden/go/code_review/updater/updater.go b/golden/go/code_review/updater/updater.go
index c49046b..8c43bf9 100644
--- a/golden/go/code_review/updater/updater.go
+++ b/golden/go/code_review/updater/updater.go
@@ -14,16 +14,14 @@
)
type Impl struct {
- crs code_review.Client
- expStore expectations.Store
- store clstore.Store
+ expStore expectations.Store
+ reviewSystems []clstore.ReviewSystem
}
-func New(c code_review.Client, e expectations.Store, s clstore.Store) *Impl {
+func New(e expectations.Store, reviewSystems []clstore.ReviewSystem) *Impl {
return &Impl{
- crs: c,
- expStore: e,
- store: s,
+ expStore: e,
+ reviewSystems: reviewSystems,
}
}
@@ -37,17 +35,24 @@
sklog.Warningf("Got more than 100 commits to update. This usually means we are starting up; We'll only check the last 100.")
commits = commits[len(commits)-100:]
}
- crs := u.crs.System()
for _, c := range commits {
- clID, err := u.crs.GetChangeListIDForCommit(ctx, c)
- if err == code_review.ErrNotFound {
+ var clID string
+ var system clstore.ReviewSystem
+ for _, rs := range u.reviewSystems {
+ // GetChangeListIDForCommit is smart enough to distinguish between two different Gerrit
+ // systems because it looks at the review URL in the CL message.
+ if id, err := rs.Client.GetChangeListIDForCommit(ctx, c); err == nil {
+ clID = id
+ system = rs
+ break
+ }
+ }
+ if clID == "" {
sklog.Warningf("Saw a commit %s that did not line up with a code review", c.Hash)
continue
}
- if err != nil {
- return skerr.Wrapf(err, "identifying the CL for %s", c.Hash)
- }
- storedCL, err := u.store.GetChangeList(ctx, clID)
+
+ storedCL, err := system.Store.GetChangeList(ctx, clID)
if err == clstore.ErrNotFound {
// Wasn't in clstore, so there was no data from TryJobs associated with that
// ChangeList, so there can't be any expectations associated with it.
@@ -61,7 +66,7 @@
continue
}
- cl, err := u.crs.GetChangeList(ctx, clID)
+ cl, err := system.Client.GetChangeList(ctx, clID)
if err == code_review.ErrNotFound {
return skerr.Fmt("somehow got an invalid CLID %s from commit %s", clID, c.Hash)
}
@@ -69,24 +74,24 @@
return skerr.Wrapf(err, "querying CRS for CL %s", c.Hash)
}
if cl.Status != code_review.Landed {
- return skerr.Fmt("cl %v of revision %s was supposed to have landed, but wasn't according to %s", cl, c.Hash, crs)
+ return skerr.Fmt("cl %v of revision %s was supposed to have landed, but wasn't according to %s", cl, c.Hash, system.ID)
}
// Write the expectations (if any) for the CL to master
- clExp := u.expStore.ForChangeList(cl.SystemID, crs)
+ clExp := u.expStore.ForChangeList(cl.SystemID, system.ID)
e, err := clExp.Get(ctx)
if err != nil {
- return skerr.Wrapf(err, "getting CLExpectations for %s (%s)", cl.SystemID, crs)
+ return skerr.Wrapf(err, "getting CLExpectations for %s (%s)", cl.SystemID, system.ID)
}
if !e.Empty() {
delta := expectations.AsDelta(e)
if err := u.expStore.AddChange(ctx, delta, cl.Owner); err != nil {
- return skerr.Wrapf(err, "writing CLExpectations for %s (%s) to master: %v", cl.SystemID, crs, e)
+ return skerr.Wrapf(err, "writing CLExpectations for %s (%s) to master: %v", cl.SystemID, system.ID, e)
}
}
// cl.Status must be Landed at this point and the CRS has set the cl's Updated time to
// the time that it was closed or marked as landed.
- if err := u.store.PutChangeList(ctx, cl); err != nil {
+ if err := system.Store.PutChangeList(ctx, cl); err != nil {
return skerr.Wrapf(err, "storing CL %v to store", cl)
}
}
diff --git a/golden/go/code_review/updater/updater_test.go b/golden/go/code_review/updater/updater_test.go
index fb8e412..8c78a10 100644
--- a/golden/go/code_review/updater/updater_test.go
+++ b/golden/go/code_review/updater/updater_test.go
@@ -30,11 +30,8 @@
mcs := &mock_clstore.Store{}
alphaExp := &mock_expectations.Store{}
betaExp := &mock_expectations.Store{}
- defer mc.AssertExpectations(t)
defer mes.AssertExpectations(t)
defer mcs.AssertExpectations(t)
- defer alphaExp.AssertExpectations(t)
- defer betaExp.AssertExpectations(t)
commits := makeCommits()
@@ -63,10 +60,8 @@
Updated: time.Date(2019, time.May, 15, 14, 18, 12, 0, time.UTC),
}, nil)
- mc.On("System").Return(crs)
-
- mes.On("ForChangeList", openCLAlpha, crs).Return(alphaExp)
- mes.On("ForChangeList", openCLBeta, crs).Return(betaExp)
+ mes.On("ForChangeList", openCLAlpha, githubCRS).Return(alphaExp)
+ mes.On("ForChangeList", openCLBeta, githubCRS).Return(betaExp)
mes.On("AddChange", testutils.AnyContext, alphaDelta, alphaAuthor).Return(nil)
mes.On("AddChange", testutils.AnyContext, betaDelta, betaAuthor).Return(nil)
@@ -99,7 +94,24 @@
}
mcs.On("PutChangeList", testutils.AnyContext, mock.MatchedBy(clChecker)).Return(nil).Twice()
- u := New(mc, mes, mcs)
+ // Pretend we are configured for Gerrit and GitHub, and GitHub doesn't recognize any of these
+ // CLs
+ gerritClient := &mock_codereview.Client{}
+ gerritClient.On("GetChangeListIDForCommit", testutils.AnyContext, mock.Anything).Return("", code_review.ErrNotFound)
+
+ u := New(mes, []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ Client: gerritClient,
+ // URLTemplate and Store not used here
+ },
+ {
+ ID: githubCRS,
+ Client: mc,
+ Store: mcs,
+ // URLTemplate not used here
+ },
+ })
err := u.UpdateChangeListsAsLanded(context.Background(), commits)
require.NoError(t, err)
}
@@ -112,10 +124,7 @@
mes := &mock_expectations.Store{}
mcs := &mock_clstore.Store{}
betaExp := &mock_expectations.Store{}
- defer mc.AssertExpectations(t)
- defer mes.AssertExpectations(t)
defer mcs.AssertExpectations(t)
- defer betaExp.AssertExpectations(t)
commits := makeCommits()[2:]
@@ -128,9 +137,8 @@
Owner: betaAuthor,
Updated: time.Date(2019, time.May, 15, 14, 18, 12, 0, time.UTC),
}, nil)
- mc.On("System").Return(crs)
- mes.On("ForChangeList", openCLBeta, crs).Return(betaExp)
+ mes.On("ForChangeList", openCLBeta, githubCRS).Return(betaExp)
betaExp.On("Get", testutils.AnyContext).Return(&betaChanges, nil)
@@ -150,7 +158,14 @@
}
mcs.On("PutChangeList", testutils.AnyContext, mock.MatchedBy(clChecker)).Return(nil).Once()
- u := New(mc, mes, mcs)
+ u := New(mes, []clstore.ReviewSystem{
+ {
+ ID: githubCRS,
+ Client: mc,
+ Store: mcs,
+ // URLTemplate not used here
+ },
+ })
err := u.UpdateChangeListsAsLanded(context.Background(), commits)
require.NoError(t, err)
}
@@ -161,44 +176,55 @@
unittest.SmallTest(t)
mc := &mock_codereview.Client{}
- mes := &mock_expectations.Store{}
mcs := &mock_clstore.Store{}
- defer mc.AssertExpectations(t)
- defer mes.AssertExpectations(t)
- defer mcs.AssertExpectations(t)
commits := makeCommits()[2:]
mc.On("GetChangeListIDForCommit", testutils.AnyContext, commits[0]).Return(openCLBeta, nil)
- mc.On("System").Return(crs)
mcs.On("GetChangeList", testutils.AnyContext, openCLBeta).Return(code_review.ChangeList{}, clstore.ErrNotFound)
- u := New(mc, mes, mcs)
+ u := New(nil, []clstore.ReviewSystem{
+ {
+ ID: githubCRS,
+ Client: mc,
+ Store: mcs,
+ // URLTemplate not used here
+ },
+ })
err := u.UpdateChangeListsAsLanded(context.Background(), commits)
require.NoError(t, err)
}
// TestUpdateNoChangeList checks the exceptional case where a commit lands without being tied to
-// a ChangeList in the CRS (we should skip it and not crash).
+// a ChangeList in any CRS (we should skip it and not crash).
func TestUpdateNoChangeList(t *testing.T) {
unittest.SmallTest(t)
mc := &mock_codereview.Client{}
- defer mc.AssertExpectations(t)
commits := makeCommits()[2:]
-
mc.On("GetChangeListIDForCommit", testutils.AnyContext, commits[0]).Return("", code_review.ErrNotFound)
- mc.On("System").Return(crs)
- u := New(mc, nil, nil)
+ u := New(nil, []clstore.ReviewSystem{
+ {
+ ID: githubCRS,
+ Client: mc,
+ // Store and URLTemplate not used here
+ },
+ {
+ ID: gerritCRS,
+ Client: mc,
+ // Store and URLTemplate not used here
+ },
+ })
err := u.UpdateChangeListsAsLanded(context.Background(), commits)
require.NoError(t, err)
}
const (
- crs = "github"
+ gerritCRS = "gerrit"
+ githubCRS = "github"
landedCL = "11196d8aff4cd689c2e49336d12928a8bd23cdec"
openCLAlpha = "aaa5f37f5bd91f1a7b3f080bf038af8e8fa4cab2"
diff --git a/golden/go/config/config.go b/golden/go/config/config.go
index 54fab72..708aea8 100644
--- a/golden/go/config/config.go
+++ b/golden/go/config/config.go
@@ -23,6 +23,10 @@
// GCP project ID that houses the BigTable Instance.
BTProjectID string `json:"bt_project_id"`
+ // One or more code review systems that we support linking to / commenting on, etc. Used also to
+ // identify valid CLs when ingesting data.
+ CodeReviewSystems []CodeReviewSystem `json:"code_review_systems"`
+
// Google Cloud Storage bucket name.
GCSBucket string `json:"gcs_bucket"`
@@ -43,9 +47,6 @@
// GCS path, where the known hashes file should be stored. Format: <bucket>/<path>.
KnownHashesGCSPath string `json:"known_hashes_gcs_path"`
- // Primary CodeReviewSystem (e.g. 'gerrit', 'github')
- PrimaryCRS string `json:"primary_crs"`
-
// If provided (e.g. ":9002"), a port serving performance-related and other debugging RPCS will
// be opened up. This RPC will not require authentication.
DebugPort string `json:"debug_port" optional:"true"`
@@ -54,6 +55,29 @@
Local bool `json:"local"`
}
+// CodeReviewSystem represents the details needed to interact with a CodeReviewSystem (e.g.
+// "gerrit", "github")
+type CodeReviewSystem struct {
+ // ID is how this CRS will be identified via query arguments and ingestion data. This is arbitrary
+ // and can be used to distinguish between and internal and public version (e.g. "gerrit-internal")
+ ID string `json:"id"`
+
+ // Specifies the APIs/code needed to interact ("gerrit", "github").
+ Flavor string `json:"flavor"`
+
+ // A URL with %s where a CL ID should be placed to complete it.
+ URLTemplate string `json:"url_template"`
+
+ // URL of the Gerrit instance (if any) where we retrieve CL metadata.
+ GerritURL string `json:"gerrit_url" optional:"true"`
+
+ // Filepath to file containing GitHub token (if this instance needs to talk to GitHub).
+ GitHubCredPath string `json:"github_cred_path" optional:"true"`
+
+ // User and repo of GitHub project to connect to (if any), e.g. google/skia
+ GitHubRepo string `json:"github_repo" optional:"true"`
+}
+
// LoadFromJSON5 reads the contents of path and tries to decode the JSON5 there into the provided
// struct. The passed in struct pointer is expected to have "json" struct tags for all fields.
// An error will be returned if any non-struct, non-bool field is its zero value *unless* it is
diff --git a/golden/go/indexer/indexer.go b/golden/go/indexer/indexer.go
index 4c921a8..feb7308 100644
--- a/golden/go/indexer/indexer.go
+++ b/golden/go/indexer/indexer.go
@@ -273,14 +273,14 @@
}
type IndexerConfig struct {
- DiffStore diff.DiffStore
ChangeListener expectations.ChangeEventRegisterer
+ DiffStore diff.DiffStore
ExpectationsStore expectations.Store
GCSClient storage.GCSClient
+ ReviewSystems []clstore.ReviewSystem
TileSource tilesource.TileSource
- Warmer warmer.DiffWarmer
TryJobStore tjstore.Store
- CLStore clstore.Store
+ Warmer warmer.DiffWarmer
}
// Indexer is the type that continuously processes data as the underlying
@@ -748,89 +748,90 @@
return
}
- // An arbitrary cut off to the amount of recent, open CLs we try to index.
- recent := now.Add(-maxAgeOfOpenCLsToIndex)
- xcl, _, err := ix.CLStore.GetChangeLists(ctx, clstore.SearchOptions{
- StartIdx: 0,
- Limit: maxCLsToIndex,
- OpenCLsOnly: true,
- After: recent,
- })
- if err != nil {
- sklog.Errorf("Could not get recent changelists: %s", err)
- return
- }
-
- sklog.Infof("Indexing %d CLs", len(xcl))
-
- crs := ix.CLStore.System()
- const numChunks = 8 // arbitrarily picked, could likely be tuned based on contention of
- // changelistCache
- chunkSize := (len(xcl) / numChunks) + 1 // add one to avoid integer truncation.
- err = util.ChunkIterParallel(ctx, len(xcl), chunkSize, func(ctx context.Context, startIdx int, endIdx int) error {
- for _, cl := range xcl[startIdx:endIdx] {
- if err := ctx.Err(); err != nil {
- sklog.Errorf("ChangeList indexing timed out (%v)", err)
- return nil
- }
-
- issueExpStore := ix.ExpectationsStore.ForChangeList(cl.SystemID, crs)
- clExps, err := issueExpStore.Get(ctx)
- if err != nil {
- return skerr.Wrapf(err, "loading expectations for cl %s (%s)", cl.SystemID, crs)
- }
- exps := expectations.Join(clExps, masterExp)
-
- clKey := fmt.Sprintf("%s_%s", crs, cl.SystemID)
- clIdx, ok := ix.getCLIndex(clKey)
- // Ingestion should update this timestamp when it has uploaded a new file belonging to this
- // changelist. We add a bit of a buffer period to avoid potential issues with a file being
- // uploaded at the exact same time we create an index (skbug.com/10265).
- updatedWithGracePeriod := cl.Updated.Add(30 * time.Second)
- if !ok || clIdx.ComputedTS.Before(updatedWithGracePeriod) {
- ix.changeListsReindexed.Inc(1)
- // Compute it from scratch and store it to the index.
- xps, err := ix.CLStore.GetPatchSets(ctx, cl.SystemID)
- if err != nil {
- return skerr.Wrap(err)
- }
- if len(xps) == 0 {
- continue
- }
- latestPS := xps[len(xps)-1]
- psID := tjstore.CombinedPSID{
- CL: cl.SystemID,
- CRS: crs,
- PS: latestPS.SystemID,
- }
- afterTime := time.Time{}
- var existingUntriagedResults []tjstore.TryJobResult
- // Test to see if we can do an incremental index (just for results that were uploaded
- // for this patchset since the last time we indexed).
- if ok && clIdx.LatestPatchSet.PS == latestPS.SystemID {
- afterTime = clIdx.ComputedTS
- existingUntriagedResults = clIdx.UntriagedResults
- }
- xtjr, err := ix.TryJobStore.GetResults(ctx, psID, afterTime)
- if err != nil {
- return skerr.Wrap(err)
- }
- untriagedResults, params := indexTryJobResults(existingUntriagedResults, xtjr, exps)
- // Copy the existing ParamSet into the newly created one. It is important to copy it from
- // old into new (and not new into old), so we don't cause a race condition on the cached
- // ParamSet by writing to it while GetIndexForCL is reading from it.
- params.AddParamSet(clIdx.ParamSet)
- clIdx.ParamSet = params
- clIdx.LatestPatchSet = psID
- clIdx.UntriagedResults = untriagedResults
- clIdx.ComputedTS = now
- }
- ix.changeListIndices.Set(clKey, &clIdx, ttlcache.DefaultExpiration)
+ for _, system := range ix.ReviewSystems {
+ // An arbitrary cut off to the amount of recent, open CLs we try to index.
+ recent := now.Add(-maxAgeOfOpenCLsToIndex)
+ xcl, _, err := system.Store.GetChangeLists(ctx, clstore.SearchOptions{
+ StartIdx: 0,
+ Limit: maxCLsToIndex,
+ OpenCLsOnly: true,
+ After: recent,
+ })
+ if err != nil {
+ sklog.Errorf("Could not get recent changelists: %s", err)
+ return
}
- return nil
- })
- if err != nil {
- sklog.Errorf("Error indexing changelists: %s", err)
+
+ sklog.Infof("Indexing %d CLs", len(xcl))
+
+ const numChunks = 8 // arbitrarily picked, could likely be tuned based on contention of
+ // changelistCache
+ chunkSize := (len(xcl) / numChunks) + 1 // add one to avoid integer truncation.
+ err = util.ChunkIterParallel(ctx, len(xcl), chunkSize, func(ctx context.Context, startIdx int, endIdx int) error {
+ for _, cl := range xcl[startIdx:endIdx] {
+ if err := ctx.Err(); err != nil {
+ sklog.Errorf("ChangeList indexing timed out (%v)", err)
+ return nil
+ }
+
+ issueExpStore := ix.ExpectationsStore.ForChangeList(cl.SystemID, system.ID)
+ clExps, err := issueExpStore.Get(ctx)
+ if err != nil {
+ return skerr.Wrapf(err, "loading expectations for cl %s (%s)", cl.SystemID, system.ID)
+ }
+ exps := expectations.Join(clExps, masterExp)
+
+ clKey := fmt.Sprintf("%s_%s", system.ID, cl.SystemID)
+ clIdx, ok := ix.getCLIndex(clKey)
+ // Ingestion should update this timestamp when it has uploaded a new file belonging to this
+ // changelist. We add a bit of a buffer period to avoid potential issues with a file being
+ // uploaded at the exact same time we create an index (skbug.com/10265).
+ updatedWithGracePeriod := cl.Updated.Add(30 * time.Second)
+ if !ok || clIdx.ComputedTS.Before(updatedWithGracePeriod) {
+ ix.changeListsReindexed.Inc(1)
+ // Compute it from scratch and store it to the index.
+ xps, err := system.Store.GetPatchSets(ctx, cl.SystemID)
+ if err != nil {
+ return skerr.Wrap(err)
+ }
+ if len(xps) == 0 {
+ continue
+ }
+ latestPS := xps[len(xps)-1]
+ psID := tjstore.CombinedPSID{
+ CL: cl.SystemID,
+ CRS: system.ID,
+ PS: latestPS.SystemID,
+ }
+ afterTime := time.Time{}
+ var existingUntriagedResults []tjstore.TryJobResult
+ // Test to see if we can do an incremental index (just for results that were uploaded
+ // for this patchset since the last time we indexed).
+ if ok && clIdx.LatestPatchSet.PS == latestPS.SystemID {
+ afterTime = clIdx.ComputedTS
+ existingUntriagedResults = clIdx.UntriagedResults
+ }
+ xtjr, err := ix.TryJobStore.GetResults(ctx, psID, afterTime)
+ if err != nil {
+ return skerr.Wrap(err)
+ }
+ untriagedResults, params := indexTryJobResults(existingUntriagedResults, xtjr, exps)
+ // Copy the existing ParamSet into the newly created one. It is important to copy it from
+ // old into new (and not new into old), so we don't cause a race condition on the cached
+ // ParamSet by writing to it while GetIndexForCL is reading from it.
+ params.AddParamSet(clIdx.ParamSet)
+ clIdx.ParamSet = params
+ clIdx.LatestPatchSet = psID
+ clIdx.UntriagedResults = untriagedResults
+ clIdx.ComputedTS = now
+ }
+ ix.changeListIndices.Set(clKey, &clIdx, ttlcache.DefaultExpiration)
+ }
+ return nil
+ })
+ if err != nil {
+ sklog.Errorf("Error indexing changelists from CRS %s: %s", system.ID, err)
+ }
}
}
diff --git a/golden/go/indexer/indexer_test.go b/golden/go/indexer/indexer_test.go
index d226d7e..e0add37 100644
--- a/golden/go/indexer/indexer_test.go
+++ b/golden/go/indexer/indexer_test.go
@@ -208,14 +208,14 @@
func TestIndexer_CalcChangeListIndices_NoPreviousIndices_Success(t *testing.T) {
unittest.SmallTest(t)
- const crs = "gerrit"
+ const gerritCRS = "gerrit"
const firstCLID = "111111"
const patchsetFoxtrot = "foxtrot"
const secondCLID = "22222"
const patchsetSam = "sam"
- firstCombinedID := tjstore.CombinedPSID{CL: firstCLID, CRS: crs, PS: patchsetFoxtrot}
- secondCombinedID := tjstore.CombinedPSID{CL: secondCLID, CRS: crs, PS: patchsetSam}
+ firstCombinedID := tjstore.CombinedPSID{CL: firstCLID, CRS: gerritCRS, PS: patchsetFoxtrot}
+ secondCombinedID := tjstore.CombinedPSID{CL: secondCLID, CRS: gerritCRS, PS: patchsetSam}
mcs := &mock_clstore.Store{}
mes := &mock_expectations.Store{}
@@ -229,7 +229,7 @@
// secondCL has no additional expectations
mes.On("Get", testutils.AnyContext).Return(&masterExp, nil)
- loadChangeListExpectations(mes, crs, map[string]*expectations.Expectations{
+ loadChangeListExpectations(mes, gerritCRS, map[string]*expectations.Expectations{
firstCLID: &firstCLExp,
secondCLID: {},
})
@@ -254,7 +254,6 @@
{SystemID: "not the most recent, so it is ignored"},
{SystemID: patchsetSam},
}, nil)
- mcs.On("System").Return(crs)
androidGroup := paramtools.Params{
"os": "Android",
@@ -304,7 +303,8 @@
ResultParams: paramtools.Params{types.PrimaryKeyField: string(data.AlphaTest)},
GroupParams: iosGroup,
Options: firstOptionalGroup,
- Digest: data.AlphaNegativeDigest, // Note, for this CL, this digest has not yet been triaged.
+ // Note, for this CL, this digest has not yet been triaged.
+ Digest: data.AlphaNegativeDigest,
},
{
ResultParams: paramtools.Params{types.PrimaryKeyField: string(data.AlphaTest)},
@@ -316,9 +316,15 @@
ctx := context.Background()
ic := IndexerConfig{
- CLStore: mcs,
ExpectationsStore: mes,
TryJobStore: mts,
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ Store: mcs,
+ // URLTemplate and Client are unused here
+ },
+ },
}
ixr, err := New(ctx, ic, 0)
require.NoError(t, err)
@@ -326,7 +332,7 @@
ixr.calcChangeListIndices(ctx)
- clIdx := ixr.GetIndexForCL(crs, firstCLID)
+ clIdx := ixr.GetIndexForCL(gerritCRS, firstCLID)
assert.NotNil(t, clIdx)
assert.Equal(t, firstCombinedID, clIdx.LatestPatchSet)
assert.Len(t, clIdx.UntriagedResults, 1)
@@ -341,7 +347,7 @@
"os": []string{"Android", "iOS"},
}, clIdx.ParamSet)
- clIdx = ixr.GetIndexForCL(crs, secondCLID)
+ clIdx = ixr.GetIndexForCL(gerritCRS, secondCLID)
assert.NotNil(t, clIdx)
assert.Equal(t, secondCombinedID, clIdx.LatestPatchSet)
assert.Len(t, clIdx.UntriagedResults, 2)
@@ -364,12 +370,12 @@
func TestIndexer_CalcChangeListIndices_HasIndexForPreviousPS_Success(t *testing.T) {
unittest.SmallTest(t)
- const crs = "gerrit"
+ const gerritCRS = "gerrit"
const clID = "111111"
const firstPatchSet = "firstPS"
const secondPatchSet = "secondPS"
- firstPatchSetCombinedID := tjstore.CombinedPSID{CL: clID, CRS: crs, PS: firstPatchSet}
- secondPatchSetCombinedID := tjstore.CombinedPSID{CL: clID, CRS: crs, PS: secondPatchSet}
+ firstPatchSetCombinedID := tjstore.CombinedPSID{CL: clID, CRS: gerritCRS, PS: firstPatchSet}
+ secondPatchSetCombinedID := tjstore.CombinedPSID{CL: clID, CRS: gerritCRS, PS: secondPatchSet}
longAgo := time.Date(2020, time.April, 15, 15, 15, 0, 0, time.UTC)
recently := time.Date(2020, time.May, 5, 12, 12, 0, 0, time.UTC)
@@ -384,7 +390,7 @@
// The CL has no additional expectations.
mes.On("Get", testutils.AnyContext).Return(&masterExp, nil)
- loadChangeListExpectations(mes, crs, map[string]*expectations.Expectations{
+ loadChangeListExpectations(mes, gerritCRS, map[string]*expectations.Expectations{
clID: {},
})
@@ -399,7 +405,6 @@
{SystemID: firstPatchSet}, // all other fields ignored from patch set.
{SystemID: secondPatchSet},
}, nil)
- mcs.On("System").Return(crs)
androidGroup := paramtools.Params{
"os": "Android",
@@ -427,9 +432,15 @@
ctx := context.Background()
ic := IndexerConfig{
- CLStore: mcs,
ExpectationsStore: mes,
TryJobStore: mts,
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ Store: mcs,
+ // URLTemplate and Client are unused here
+ },
+ },
}
ixr, err := New(ctx, ic, 0)
require.NoError(t, err)
@@ -469,7 +480,7 @@
ixr.calcChangeListIndices(ctx)
- clIdx := ixr.GetIndexForCL(crs, clID)
+ clIdx := ixr.GetIndexForCL(gerritCRS, clID)
assert.NotNil(t, clIdx)
assert.Equal(t, secondPatchSetCombinedID, clIdx.LatestPatchSet)
assert.True(t, clIdx.ComputedTS.After(longAgo)) // should be updated
@@ -488,14 +499,14 @@
func TestIndexer_CalcChangeListIndices_HasIndexForCurrentPS_IncrementalUpdateSuccess(t *testing.T) {
unittest.SmallTest(t)
- const crs = "gerrit"
+ const gerritCRS = "gerrit"
const clID = "111111"
const firstPatchSet = "firstPS"
const firstUntriagedDigest = types.Digest("11111111111111111111")
const secondUntriagedDigest = types.Digest("22222222222222222222")
const thirdUntriagedDigest = types.Digest("33333333333333333333")
- firstPatchSetCombinedID := tjstore.CombinedPSID{CL: clID, CRS: crs, PS: firstPatchSet}
+ firstPatchSetCombinedID := tjstore.CombinedPSID{CL: clID, CRS: gerritCRS, PS: firstPatchSet}
longAgo := time.Date(2020, time.April, 15, 15, 15, 0, 0, time.UTC)
recently := time.Date(2020, time.May, 5, 12, 12, 0, 0, time.UTC)
@@ -509,7 +520,7 @@
// The CL has no additional expectations.
mes.On("Get", testutils.AnyContext).Return(&masterExp, nil)
- loadChangeListExpectations(mes, crs, map[string]*expectations.Expectations{
+ loadChangeListExpectations(mes, gerritCRS, map[string]*expectations.Expectations{
clID: {},
})
@@ -523,7 +534,6 @@
mcs.On("GetPatchSets", testutils.AnyContext, clID).Return([]code_review.PatchSet{
{SystemID: firstPatchSet}, // all other fields ignored from patch set.
}, nil)
- mcs.On("System").Return(crs)
androidGroup := paramtools.Params{
"os": "Android",
@@ -546,9 +556,15 @@
ctx := context.Background()
ic := IndexerConfig{
- CLStore: mcs,
ExpectationsStore: mes,
TryJobStore: mts,
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ Store: mcs,
+ // URLTemplate and Client are unused here
+ },
+ },
}
ixr, err := New(ctx, ic, 0)
require.NoError(t, err)
@@ -582,7 +598,7 @@
ixr.calcChangeListIndices(ctx)
- clIdx := ixr.GetIndexForCL(crs, clID)
+ clIdx := ixr.GetIndexForCL(gerritCRS, clID)
assert.NotNil(t, clIdx)
assert.Equal(t, firstPatchSetCombinedID, clIdx.LatestPatchSet)
assert.True(t, clIdx.ComputedTS.After(longAgo)) // should be updated
@@ -603,10 +619,10 @@
func TestIndexer_CalcChangeListIndices_PreviousIndexDoesNotNeedUpdating_Success(t *testing.T) {
unittest.SmallTest(t)
- const crs = "gerrit"
+ const gerritCRS = "gerrit"
const clID = "111111"
const thePatchSet = "firstPS"
- thePatchSetCombinedID := tjstore.CombinedPSID{CL: clID, CRS: crs, PS: thePatchSet}
+ thePatchSetCombinedID := tjstore.CombinedPSID{CL: clID, CRS: gerritCRS, PS: thePatchSet}
now := time.Date(2020, time.May, 15, 15, 15, 0, 0, time.UTC)
fiveMinAgo := now.Add(-5 * time.Minute)
@@ -621,7 +637,7 @@
// The CL has no additional expectations.
mes.On("Get", testutils.AnyContext).Return(&masterExp, nil)
- loadChangeListExpectations(mes, crs, map[string]*expectations.Expectations{
+ loadChangeListExpectations(mes, gerritCRS, map[string]*expectations.Expectations{
clID: {},
})
@@ -637,12 +653,17 @@
mcs.On("GetPatchSets", testutils.AnyContext, clID).Return([]code_review.PatchSet{
{SystemID: thePatchSet}, // all other fields ignored from patch set.
}, nil)
- mcs.On("System").Return(crs)
ctx := context.Background()
ic := IndexerConfig{
- CLStore: mcs,
ExpectationsStore: mes,
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ Store: mcs,
+ // URLTemplate and Client are unused here
+ },
+ },
}
ixr, err := New(ctx, ic, 0)
require.NoError(t, err)
@@ -672,7 +693,7 @@
ixr.calcChangeListIndices(ctx)
- clIdx := ixr.GetIndexForCL(crs, clID)
+ clIdx := ixr.GetIndexForCL(gerritCRS, clID)
assert.NotNil(t, clIdx)
assert.Equal(t, thePatchSetCombinedID, clIdx.LatestPatchSet)
assert.Equal(t, clIdx.ComputedTS, fiveMinAgo) // should not be updated
diff --git a/golden/go/ingestion_processors/tryjob_ingestion.go b/golden/go/ingestion_processors/tryjob_ingestion.go
index 8b13df0..30a2470 100644
--- a/golden/go/ingestion_processors/tryjob_ingestion.go
+++ b/golden/go/ingestion_processors/tryjob_ingestion.go
@@ -38,17 +38,19 @@
firestoreProjectIDParam = "FirestoreProjectID"
firestoreNamespaceParam = "FirestoreNamespace"
- codeReviewSystemParam = "CodeReviewSystem"
+ codeReviewSystemsParam = "CodeReviewSystems"
gerritURLParam = "GerritURL"
+ gerritInternalURLParam = "GerritInternalURL"
githubRepoParam = "GitHubRepo"
githubCredentialsPathParam = "GitHubCredentialsPath"
continuousIntegrationSystemsParam = "ContinuousIntegrationSystems"
- gerritCRS = "gerrit"
- githubCRS = "github"
- buildbucketCIS = "buildbucket"
- cirrusCIS = "cirrus"
+ gerritCRS = "gerrit"
+ gerritInternalCRS = "gerrit-internal"
+ githubCRS = "github"
+ buildbucketCIS = "buildbucket"
+ cirrusCIS = "cirrus"
)
// Register the ingestion Processor with the ingestion framework.
@@ -58,29 +60,15 @@
// goldTryjobProcessor implements the ingestion.Processor interface to ingest tryjob results.
type goldTryjobProcessor struct {
- reviewClient code_review.Client
- cisClients map[string]continuous_integration.Client
-
- changeListStore clstore.Store
- tryJobStore tjstore.Store
-
- crsName string
+ cisClients map[string]continuous_integration.Client
+ reviewSystems []clstore.ReviewSystem
+ tryJobStore tjstore.Store
}
// newModularTryjobProcessor returns an ingestion.Processor which is modular and can support
// different CodeReviewSystems (e.g. "Gerrit", "GitHub") and different ContinuousIntegrationSystems
// (e.g. "BuildBucket", "CirrusCI"). This particular implementation stores the data in Firestore.
func newModularTryjobProcessor(ctx context.Context, _ vcsinfo.VCS, config ingestion.Config, client *http.Client) (ingestion.Processor, error) {
- crsName := config.ExtraParams[codeReviewSystemParam]
- if strings.TrimSpace(crsName) == "" {
- return nil, skerr.Fmt("missing code review system (e.g. 'gerrit')")
- }
-
- crs, err := codeReviewSystemFactory(crsName, config, client)
- if err != nil {
- return nil, skerr.Wrapf(err, "could not create client for CRS %q", crsName)
- }
-
cisNames := strings.Split(config.ExtraParams[continuousIntegrationSystemsParam], ",")
if len(cisNames) == 0 {
return nil, skerr.Fmt("missing CI system (e.g. 'buildbucket')")
@@ -114,12 +102,28 @@
return nil, skerr.Wrapf(err, "initializing expectation store")
}
+ crsNames := strings.Split(config.ExtraParams[codeReviewSystemsParam], ",")
+ if len(crsNames) == 0 {
+ return nil, skerr.Fmt("missing CRS (e.g. 'gerrit')")
+ }
+
+ var reviewSystems []clstore.ReviewSystem
+ for _, crsName := range crsNames {
+ crsClient, err := codeReviewSystemFactory(crsName, config, client)
+ if err != nil {
+ return nil, skerr.Wrapf(err, "could not create client for CRS %q", crsName)
+ }
+ reviewSystems = append(reviewSystems, clstore.ReviewSystem{
+ ID: crsName,
+ Client: crsClient,
+ Store: fs_clstore.New(fsClient, crsName),
+ })
+ }
+
return &goldTryjobProcessor{
- reviewClient: crs,
- cisClients: cisClients,
- changeListStore: fs_clstore.New(fsClient, crsName),
- tryJobStore: fs_tjstore.New(fsClient),
- crsName: crsName,
+ cisClients: cisClients,
+ tryJobStore: fs_tjstore.New(fsClient),
+ reviewSystems: reviewSystems,
}, nil
}
@@ -135,6 +139,17 @@
}
return gerrit_crs.New(gerritClient), nil
}
+ if crsName == gerritInternalCRS {
+ gerritURL := config.ExtraParams[gerritInternalURLParam]
+ if strings.TrimSpace(gerritURL) == "" {
+ return nil, skerr.Fmt("missing URL for the Gerrit internal code review system")
+ }
+ gerritClient, err := gerrit.NewGerrit(gerritURL, client)
+ if err != nil {
+ return nil, skerr.Wrapf(err, "creating gerrit client for %s", gerritURL)
+ }
+ return gerrit_crs.New(gerritClient), nil
+ }
if crsName == githubCRS {
githubRepo := config.ExtraParams[githubRepoParam]
if strings.TrimSpace(githubRepo) == "" {
@@ -181,24 +196,25 @@
psID := ""
crs := gr.CodeReviewSystem
if crs == "" {
- // Default to Gerrit
+ // Default to Gerrit; TODO(kjlubick) who uses this?
+ sklog.Warningf("Using default CRS (this may go away soon)")
crs = gerritCRS
}
- if crs == g.crsName {
- clID = gr.ChangeListID
- psOrder = gr.PatchSetOrder
- psID = gr.PatchSetID
- } else {
- sklog.Warningf("Result %s said it was for crs %q, but this ingester is configured for %s", rf.Name(), crs, g.crsName)
- // We only support one CRS and one CIS at the moment, but if needed, we can have
- // multiple configured and pivot to the one we need.
+
+ system, ok := g.getCodeReviewSystem(crs)
+ if !ok {
+ sklog.Warningf("Result %s said it was for crs %q, which we aren't configured for", rf.Name(), crs)
return ingestion.IgnoreResultsFileErr
}
+ clID = gr.ChangeListID
+ psOrder = gr.PatchSetOrder
+ psID = gr.PatchSetID
tjID := ""
cisName := gr.ContinuousIntegrationSystem
if cisName == "" {
- // Default to BuildBucket
+ // Default to BuildBucket; TODO(kjlubick) who uses this?
+ sklog.Warningf("Using default CIS (this may go away soon)")
cisName = buildbucketCIS
}
var cisClient continuous_integration.Client
@@ -213,9 +229,9 @@
}
// Fetch CL from clstore if we have seen it before, from CRS if we have not.
- cl, err := g.changeListStore.GetChangeList(ctx, clID)
+ cl, err := system.Store.GetChangeList(ctx, clID)
if err == clstore.ErrNotFound {
- cl, err = g.reviewClient.GetChangeList(ctx, clID)
+ cl, err = system.Client.GetChangeList(ctx, clID)
if err == code_review.ErrNotFound {
sklog.Warningf("Unknown %s CL with id %q", crs, clID)
// Try again later - maybe the input was created before the CL?
@@ -229,7 +245,7 @@
return skerr.Wrapf(err, "fetching CL from clstore with id %q", clID)
}
- ps, err := g.getPatchSet(ctx, psOrder, psID, clID, crs)
+ ps, err := g.getPatchSet(ctx, system, psOrder, psID, clID)
if err != nil {
// Do not wrap this error - this returns IgnoreResultsFileErr sometimes.
return err
@@ -273,7 +289,7 @@
// Store the results from the file.
tjr := toTryJobResults(gr)
- if err := g.changeListStore.PutPatchSet(ctx, ps); err != nil {
+ if err := system.Store.PutPatchSet(ctx, ps); err != nil {
return skerr.Wrapf(err, "could not store PS %s of CL %q to clstore", psID, clID)
}
err = g.tryJobStore.PutResults(ctx, combinedID, tjID, cisName, tjr, time.Now())
@@ -285,7 +301,7 @@
// to determine if any changes have happened to the CL or any children PSes in a given time
// period.
cl.Updated = time.Now()
- if err = g.changeListStore.PutChangeList(ctx, cl); err != nil {
+ if err = system.Store.PutChangeList(ctx, cl); err != nil {
return skerr.Wrapf(err, "updating CL with id %q to clstore", clID)
}
return nil
@@ -293,15 +309,15 @@
// getPatchSet looks up a PatchSet either by id or order from our changeListStore. If it's not
// there, it looks it up from the CRS and then stores it to the changeListStore before returning it.
-func (g *goldTryjobProcessor) getPatchSet(ctx context.Context, psOrder int, psID, clID, crs string) (code_review.PatchSet, error) {
+func (g *goldTryjobProcessor) getPatchSet(ctx context.Context, system clstore.ReviewSystem, psOrder int, psID, clID string) (code_review.PatchSet, error) {
// Try looking up patchset by ID first, then fall back to order.
if psID != "" {
// Fetch PS from clstore if we have seen it before, from CRS if we have not.
- ps, err := g.changeListStore.GetPatchSet(ctx, clID, psID)
+ ps, err := system.Store.GetPatchSet(ctx, clID, psID)
if err == clstore.ErrNotFound {
- xps, err := g.reviewClient.GetPatchSets(ctx, clID)
+ xps, err := system.Client.GetPatchSets(ctx, clID)
if err != nil {
- return code_review.PatchSet{}, skerr.Wrapf(err, "could not get patchsets for %s cl %s", crs, clID)
+ return code_review.PatchSet{}, skerr.Wrapf(err, "could not get patchsets for %s cl %s", system.ID, clID)
}
// It should be ok to overwrite any PatchSets we've seen before - they should be
// immutable.
@@ -310,7 +326,7 @@
return p, nil
}
}
- sklog.Warningf("Unknown %s PS %s for CL %q", crs, psID, clID)
+ sklog.Warningf("Unknown %s PS %s for CL %q", system.ID, psID, clID)
// Try again later - maybe the input was created before the CL uploaded its PS?
return code_review.PatchSet{}, ingestion.IgnoreResultsFileErr
@@ -321,11 +337,11 @@
return ps, nil
}
// Fetch PS from clstore if we have seen it before, from CRS if we have not.
- ps, err := g.changeListStore.GetPatchSetByOrder(ctx, clID, psOrder)
+ ps, err := system.Store.GetPatchSetByOrder(ctx, clID, psOrder)
if err == clstore.ErrNotFound {
- xps, err := g.reviewClient.GetPatchSets(ctx, clID)
+ xps, err := system.Client.GetPatchSets(ctx, clID)
if err != nil {
- return code_review.PatchSet{}, skerr.Wrapf(err, "could not get patchsets for %s cl %s", crs, clID)
+ return code_review.PatchSet{}, skerr.Wrapf(err, "could not get patchsets for %s cl %s", system.ID, clID)
}
// It should be ok to put any PatchSets we've seen before - they should be immutable.
for _, p := range xps {
@@ -333,7 +349,7 @@
return p, nil
}
}
- sklog.Warningf("Unknown %s PS with order %d for CL %q", crs, psOrder, clID)
+ sklog.Warningf("Unknown %s PS with order %d for CL %q", system.ID, psOrder, clID)
// Try again later - maybe the input was created before the CL uploaded its PS?
return code_review.PatchSet{}, ingestion.IgnoreResultsFileErr
} else if err != nil {
@@ -356,3 +372,17 @@
}
return tjr
}
+
+// getCodeReviewSystem returns the ReviewSystem associated with the crs, or false if there was no
+// match.
+func (g *goldTryjobProcessor) getCodeReviewSystem(crs string) (clstore.ReviewSystem, bool) {
+ var system clstore.ReviewSystem
+ found := false
+ for _, rs := range g.reviewSystems {
+ if rs.ID == crs {
+ system = rs
+ found = true
+ }
+ }
+ return system, found
+}
diff --git a/golden/go/ingestion_processors/tryjob_ingestion_test.go b/golden/go/ingestion_processors/tryjob_ingestion_test.go
index 0c3bb70..d315513 100644
--- a/golden/go/ingestion_processors/tryjob_ingestion_test.go
+++ b/golden/go/ingestion_processors/tryjob_ingestion_test.go
@@ -42,8 +42,9 @@
firestoreProjectIDParam: "should-use-emulator",
firestoreNamespaceParam: "testing",
- codeReviewSystemParam: "gerrit",
- gerritURLParam: "https://example-review.googlesource.com",
+ codeReviewSystemsParam: "gerrit,gerrit-internal",
+ gerritURLParam: "https://example-review.googlesource.com",
+ gerritInternalURLParam: "https://example-internal-review.googlesource.com",
continuousIntegrationSystemsParam: "buildbucket",
},
@@ -55,7 +56,7 @@
gtp, ok := p.(*goldTryjobProcessor)
require.True(t, ok)
- assert.NotNil(t, gtp.reviewClient)
+ assert.Len(t, gtp.reviewSystems, 2)
assert.Len(t, gtp.cisClients, 1)
assert.Contains(t, gtp.cisClients, buildbucketCIS)
}
@@ -68,7 +69,7 @@
firestoreProjectIDParam: "should-use-emulator",
firestoreNamespaceParam: "testing",
- codeReviewSystemParam: "github",
+ codeReviewSystemsParam: "github",
githubRepoParam: "google/skia",
githubCredentialsPathParam: "testdata/fake_token", // this is actually a file on disk.
@@ -82,7 +83,7 @@
gtp, ok := p.(*goldTryjobProcessor)
require.True(t, ok)
- assert.NotNil(t, gtp.reviewClient)
+ assert.Len(t, gtp.reviewSystems, 1)
assert.Len(t, gtp.cisClients, 2)
assert.Contains(t, gtp.cisClients, cirrusCIS)
assert.Contains(t, gtp.cisClients, buildbucketCIS)
@@ -105,11 +106,16 @@
mtjs.On("PutResults", testutils.AnyContext, gerritCombinedID, gerritTJID, buildbucketCIS, makeTryJobResults(), anyTime).Return(nil).Once()
gtp := goldTryjobProcessor{
- changeListStore: mcls,
- crsName: gerritCRS,
- cisClients: makeBuildbucketCIS(),
- reviewClient: makeGerritCRS(),
- tryJobStore: mtjs,
+ cisClients: makeBuildbucketCIS(),
+ reviewSystems: []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ Client: makeGerritCRS(),
+ Store: mcls,
+ // URLTemplate unused here
+ },
+ },
+ tryJobStore: mtjs,
}
fsResult, err := ingestion_mocks.MockResultFileLocationFromFile(legacyGoldCtlFile)
@@ -146,11 +152,16 @@
mtjs.On("PutResults", testutils.AnyContext, combinedID, githubTJID, cirrusCIS, makeGitHubTryJobResults(), anyTime).Return(nil)
gtp := goldTryjobProcessor{
- changeListStore: mcls,
- crsName: githubCRS,
- cisClients: makeCirrusCIS(),
- reviewClient: makeGitHubCRS(),
- tryJobStore: mtjs,
+ cisClients: makeCirrusCIS(),
+ reviewSystems: []clstore.ReviewSystem{
+ {
+ ID: githubCRS,
+ Client: makeGitHubCRS(),
+ Store: mcls,
+ // URLTemplate unused here
+ },
+ },
+ tryJobStore: mtjs,
}
fsResult, err := ingestion_mocks.MockResultFileLocationFromFile(githubGoldCtlFile)
@@ -184,10 +195,15 @@
}
gtp := goldTryjobProcessor{
- changeListStore: mcls,
- crsName: githubCRS,
- cisClients: errorfulCISClients,
- tryJobStore: makeEmptyTJStore(),
+ cisClients: errorfulCISClients,
+ reviewSystems: []clstore.ReviewSystem{
+ {
+ ID: githubCRS,
+ Store: mcls,
+ // Client, URLTemplate unused here
+ },
+ },
+ tryJobStore: makeEmptyTJStore(),
}
err := gtp.Process(context.Background(), githubIngestionResultFromCIS(t, buildbucketCIS))
@@ -256,11 +272,16 @@
mtjs.On("PutResults", testutils.AnyContext, gerritCombinedID, gerritTJID, buildbucketCIS, makeTryJobResults(), anyTime).Return(nil)
gtp := goldTryjobProcessor{
- changeListStore: mcls,
- crsName: gerritCRS,
- cisClients: makeBuildbucketCIS(),
- reviewClient: makeGerritCRS(),
- tryJobStore: mtjs,
+ cisClients: makeBuildbucketCIS(),
+ reviewSystems: []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ Client: makeGerritCRS(),
+ Store: mcls,
+ // URLTemplate unused here
+ },
+ },
+ tryJobStore: mtjs,
}
fsResult, err := ingestion_mocks.MockResultFileLocationFromFile(legacyGoldCtlFile)
@@ -293,11 +314,16 @@
mtjs.On("PutResults", testutils.AnyContext, gerritCombinedID, gerritTJID, buildbucketCIS, makeTryJobResults(), anyTime).Return(nil)
gtp := goldTryjobProcessor{
- changeListStore: mcls,
- crsName: gerritCRS,
- cisClients: makeBuildbucketCIS(),
- reviewClient: makeGerritCRS(),
- tryJobStore: mtjs,
+ cisClients: makeBuildbucketCIS(),
+ reviewSystems: []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ Client: makeGerritCRS(),
+ Store: mcls,
+ // URLTemplate unused here
+ },
+ },
+ tryJobStore: mtjs,
}
fsResult, err := ingestion_mocks.MockResultFileLocationFromFile(legacyGoldCtlFile)
@@ -327,10 +353,15 @@
mtjs.On("PutResults", testutils.AnyContext, gerritCombinedID, gerritTJID, buildbucketCIS, makeTryJobResults(), anyTime).Return(nil)
gtp := goldTryjobProcessor{
- changeListStore: mcls,
- crsName: gerritCRS,
- cisClients: makeBuildbucketCIS(),
- tryJobStore: mtjs,
+ cisClients: makeBuildbucketCIS(),
+ reviewSystems: []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ Store: mcls,
+ // Client, URLTemplate unused here
+ },
+ },
+ tryJobStore: mtjs,
}
fsResult, err := ingestion_mocks.MockResultFileLocationFromFile(legacyGoldCtlFile)
diff --git a/golden/go/search/benchmarks_test.go b/golden/go/search/benchmarks_test.go
index 9aad772..26d37d0 100644
--- a/golden/go/search/benchmarks_test.go
+++ b/golden/go/search/benchmarks_test.go
@@ -8,15 +8,16 @@
"testing"
"github.com/stretchr/testify/require"
- "go.skia.org/infra/golden/go/tiling"
"go.skia.org/infra/go/paramtools"
"go.skia.org/infra/go/testutils"
+ "go.skia.org/infra/golden/go/clstore"
mock_clstore "go.skia.org/infra/golden/go/clstore/mocks"
"go.skia.org/infra/golden/go/code_review"
"go.skia.org/infra/golden/go/expectations"
mock_index "go.skia.org/infra/golden/go/indexer/mocks"
"go.skia.org/infra/golden/go/search/query"
+ "go.skia.org/infra/golden/go/tiling"
"go.skia.org/infra/golden/go/tjstore"
mock_tjstore "go.skia.org/infra/golden/go/tjstore/mocks"
"go.skia.org/infra/golden/go/types"
@@ -36,6 +37,7 @@
// BenchmarkExtractChangeListDigests benchmarks extractChangeListDigests, specifically
// focusing on the filtering logic after the TryJobResults are returned.
func BenchmarkExtractChangeListDigests(b *testing.B) {
+ const gerritCRS = "gerrit"
mis := &mock_index.IndexSearcher{}
mcls := &mock_clstore.Store{}
mtjs := &mock_tjstore.Store{}
@@ -54,7 +56,6 @@
// All the rest are ignored
},
}, nil)
- mcls.On("System").Return("gerrit")
mis.On("GetIgnoreMatcher").Return(makeIgnoreRules())
combinedID := tjstore.CombinedPSID{CL: "123", CRS: "gerrit", PS: "first_one"}
@@ -65,9 +66,17 @@
return c
}, nil)
+ reviewSystems := []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ Store: mcls,
+ // Client and URLTemplate are unused here
+ },
+ }
+
s := &SearchImpl{
- changeListStore: mcls,
- tryJobStore: mtjs,
+ reviewSystems: reviewSystems,
+ tryJobStore: mtjs,
}
fn := func(_ types.TestName, _ types.Digest, _ paramtools.Params, _ tiling.TracePair) {}
@@ -78,6 +87,7 @@
PatchSets: []int64{int64(psOrder)},
TraceValues: map[string][]string{},
IncludeUntriagedDigests: true,
+ CodeReviewSystemID: "gerrit",
ChangeListID: clID,
}, mis, expectations.EmptyClassifier(), fn)
require.NoError(b, err)
diff --git a/golden/go/search/query/types.go b/golden/go/search/query/types.go
index f0c3648..7a3fb6f 100644
--- a/golden/go/search/query/types.go
+++ b/golden/go/search/query/types.go
@@ -32,7 +32,8 @@
RightTraceValues paramtools.ParamSet `json:"-"`
// TryJob support.
- ChangeListID string `json:"issue"`
+ ChangeListID string `json:"issue"`
+ CodeReviewSystemID string `json:"crs_id"`
// TODO(kjlubick) Change this so only one patchset is allowed. It will simplify the backend code.
PatchSetsStr string `json:"patchsets"` // Comma-separated list of patchsets.
PatchSets []int64 `json:"-"`
diff --git a/golden/go/search/search.go b/golden/go/search/search.go
index ba5a8dc..66c57a3 100644
--- a/golden/go/search/search.go
+++ b/golden/go/search/search.go
@@ -5,6 +5,7 @@
import (
"context"
"sort"
+ "strings"
"sync"
"time"
@@ -53,7 +54,7 @@
diffStore diff.DiffStore
expectationsStore expectations.Store
indexSource indexer.IndexSource
- changeListStore clstore.Store
+ reviewSystems []clstore.ReviewSystem
tryJobStore tjstore.Store
commentStore comment.Store
@@ -79,7 +80,7 @@
}
// New returns a new SearchImpl instance.
-func New(ds diff.DiffStore, es expectations.Store, cer expectations.ChangeEventRegisterer, is indexer.IndexSource, cls clstore.Store, tjs tjstore.Store, cs comment.Store, publiclyViewableParams publicparams.Matcher, flakyThreshold int) *SearchImpl {
+func New(ds diff.DiffStore, es expectations.Store, cer expectations.ChangeEventRegisterer, is indexer.IndexSource, reviewSystems []clstore.ReviewSystem, tjs tjstore.Store, cs comment.Store, publiclyViewableParams publicparams.Matcher, flakyThreshold int) *SearchImpl {
var triageHistoryCache sync.Map
if cer != nil {
// If the expectations change for a given ID, we should purge it from our cache so as not
@@ -93,7 +94,7 @@
diffStore: ds,
expectationsStore: es,
indexSource: is,
- changeListStore: cls,
+ reviewSystems: reviewSystems,
tryJobStore: tjs,
commentStore: cs,
publiclyViewableParams: publiclyViewableParams,
@@ -122,9 +123,10 @@
isChangeListSearch := q.ChangeListID != "" && q.ChangeListID != "0"
// Get the expectations and the current index, which we assume constant
// for the duration of this query.
- crs := ""
- if s.changeListStore != nil {
- crs = s.changeListStore.System()
+ crs := q.CodeReviewSystemID
+ if isChangeListSearch && crs == "" {
+ // TODO(kjlubick) remove this default after the search page is converted to lit-html.
+ crs = s.reviewSystems[0].ID
}
exp, err := s.getExpectations(ctx, q.ChangeListID, crs)
if err != nil {
@@ -136,7 +138,11 @@
var results []*frontend.SearchResult
// Find the digests (left hand side) we are interested in.
if isChangeListSearch {
- cl, err := s.changeListStore.GetChangeList(ctx, q.ChangeListID)
+ reviewSystem, err := s.reviewSystem(crs)
+ if err != nil {
+ return nil, skerr.Wrap(err)
+ }
+ cl, err := reviewSystem.Store.GetChangeList(ctx, q.ChangeListID)
if err != nil {
return nil, skerr.Wrap(err)
}
@@ -144,11 +150,11 @@
// the trace data, which will include this CL's output appended to the end (as if it was the
// most recent commit to land on master).
commits = append(commits, web_frontend.Commit{
- CommitTime: cl.Updated.Unix(),
- Hash: cl.SystemID,
- Author: cl.Owner,
- Subject: cl.Subject,
- IsChangeList: true,
+ CommitTime: cl.Updated.Unix(),
+ Hash: cl.SystemID,
+ Author: cl.Owner,
+ Subject: cl.Subject,
+ ChangeListURL: strings.Replace(reviewSystem.URLTemplate, "%s", cl.SystemID, 1),
})
results, err = s.queryChangeList(ctx, q, idx, exp)
if err != nil {
@@ -334,7 +340,7 @@
}
// We know xps is sorted by order, if it is non-nil.
- xps, err := s.getPatchSets(ctx, clID)
+ xps, err := s.getPatchSets(ctx, crs, clID)
if err != nil {
return nil, skerr.Wrapf(err, "getting PatchSets for CL %s", clID)
}
@@ -455,7 +461,7 @@
clID := q.ChangeListID
// We know xps is sorted by order, if it is non-nil.
- xps, err := s.getPatchSets(ctx, clID)
+ xps, err := s.getPatchSets(ctx, q.CodeReviewSystemID, clID)
if err != nil {
return skerr.Wrapf(err, "getting PatchSets for CL %s", clID)
}
@@ -485,7 +491,7 @@
id := tjstore.CombinedPSID{
CL: ps.ChangeListID,
- CRS: s.changeListStore.System(),
+ CRS: q.CodeReviewSystemID,
PS: ps.SystemID,
}
@@ -570,12 +576,16 @@
}
// getPatchSets returns the PatchSets for a given CL either from the store or from the cache.
-func (s *SearchImpl) getPatchSets(ctx context.Context, id string) ([]code_review.PatchSet, error) {
- key := "patchsets_" + id
+func (s *SearchImpl) getPatchSets(ctx context.Context, crs, id string) ([]code_review.PatchSet, error) {
+ key := crs + "_patchsets_" + id
if xtr, ok := s.storeCache.Get(key); ok {
return xtr.([]code_review.PatchSet), nil
}
- xps, err := s.changeListStore.GetPatchSets(ctx, id)
+ rs, err := s.reviewSystem(crs)
+ if err != nil {
+ return nil, skerr.Wrap(err)
+ }
+ xps, err := rs.Store.GetPatchSets(ctx, id)
if err != nil {
return nil, skerr.Wrap(err)
}
@@ -1161,5 +1171,15 @@
wg.Wait()
}
+func (s *SearchImpl) reviewSystem(crs string) (clstore.ReviewSystem, error) {
+ for _, rs := range s.reviewSystems {
+ if rs.ID == crs {
+ return rs, nil
+ }
+ }
+ sklog.Errorf("Got passed in an unknown crs - %q", crs)
+ return clstore.ReviewSystem{}, skerr.Fmt("Invalid crs")
+}
+
// Make sure SearchImpl fulfills the SearchAPI interface.
var _ SearchAPI = (*SearchImpl)(nil)
diff --git a/golden/go/search/search_test.go b/golden/go/search/search_test.go
index bf844a8..b465513 100644
--- a/golden/go/search/search_test.go
+++ b/golden/go/search/search_test.go
@@ -12,11 +12,12 @@
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
- "go.skia.org/infra/go/metrics2"
+ "go.skia.org/infra/go/metrics2"
"go.skia.org/infra/go/paramtools"
"go.skia.org/infra/go/testutils"
"go.skia.org/infra/go/testutils/unittest"
+ "go.skia.org/infra/golden/go/clstore"
mock_clstore "go.skia.org/infra/golden/go/clstore/mocks"
"go.skia.org/infra/golden/go/code_review"
"go.skia.org/infra/golden/go/comment"
@@ -748,7 +749,6 @@
// All the rest are ignored
},
}, nil).Once() // this should be cached after fetch, as it could be expensive to retrieve.
- mcls.On("System").Return(crs)
expectedID := tjstore.CombinedPSID{
CL: clID,
@@ -807,9 +807,28 @@
mds := &mock_diffstore.DiffStore{}
addDiffData(mds, BetaBrandNewDigest, data.BetaPositiveDigest, makeSmallDiffMetric())
- s := New(mds, mes, nil, makeThreeDevicesIndexer(), mcls, mtjs, nil, everythingPublic, nothingFlaky)
+ const gerritCRS = "gerrit"
+ const gerritInternalCRS = "gerrit-internal"
+
+ reviewSystems := []clstore.ReviewSystem{
+ {
+ ID: gerritInternalCRS,
+ Store: nil, // nil to catch errors if this is used (when it shouldn't be)
+ URLTemplate: "Should not be used",
+ // Client is unused here
+ },
+ {
+ ID: gerritCRS,
+ Store: mcls,
+ URLTemplate: "https://skia-review.googlesource.com/%s",
+ // Client is unused here
+ },
+ }
+
+ s := New(mds, mes, nil, makeThreeDevicesIndexer(), reviewSystems, mtjs, nil, everythingPublic, nothingFlaky)
q := &query.Search{
+ CodeReviewSystemID: gerritCRS,
ChangeListID: clID,
IncludeDigestsProducedOnMaster: false,
@@ -834,11 +853,11 @@
// We expect to see the current CL appended to the list of master branch commits.
masterBranchCommits := web_frontend.FromTilingCommits(data.MakeTestCommits())
masterBranchCommitsWithCL := append(masterBranchCommits, web_frontend.Commit{
- CommitTime: clTime.Unix(),
- Hash: clID,
- Author: clAuthor,
- Subject: clSubject,
- IsChangeList: true,
+ CommitTime: clTime.Unix(),
+ Hash: clID,
+ Author: clAuthor,
+ Subject: clSubject,
+ ChangeListURL: "https://skia-review.googlesource.com/1234",
})
assert.Equal(t, &frontend.SearchResponse{
Commits: masterBranchCommitsWithCL,
@@ -938,7 +957,6 @@
// other fields are ignored
},
}, nil)
- mcs.On("System").Return(crs)
anglerGroup := map[string]string{
"device": data.AnglerDevice,
@@ -984,16 +1002,23 @@
mis.On("Tile").Return(emptyMasterTile)
s := SearchImpl{
- indexSource: mi,
- changeListStore: mcs,
- tryJobStore: nil, // we should not actually hit the TryJobStore, because the cache was used.
- storeCache: ttlcache.New(0, 0),
+ indexSource: mi,
+ reviewSystems: []clstore.ReviewSystem{
+ {
+ ID: crs,
+ Store: mcs,
+ // Client and URLTemplate are unused here
+ },
+ },
+ tryJobStore: nil, // we should not actually hit the TryJobStore, because the cache was used.
+ storeCache: ttlcache.New(0, 0),
clIndexCacheHitCounter: metrics2.GetCounter("hit"),
clIndexCacheMissCounter: metrics2.GetCounter("miss"),
}
q := &query.Search{
+ CodeReviewSystemID: crs,
ChangeListID: clID,
IncludeDigestsProducedOnMaster: false,
@@ -1299,7 +1324,16 @@
},
}, nil)
- s := New(nil, mes, nil, mis, mcs, mts, nil, everythingPublic, nothingFlaky)
+ reviewSystems := []clstore.ReviewSystem{
+ {
+ ID: testCRS,
+ Store: mcs,
+ URLTemplate: "https://skia-review.googlesource.com/%s",
+ // Client and URLTemplate are unused here
+ },
+ }
+
+ s := New(nil, mes, nil, mis, reviewSystems, mts, nil, everythingPublic, nothingFlaky)
rv, err := s.GetDigestDetails(context.Background(), testWeWantDetailsAbout, digestWeWantDetailsAbout, testCLID, testCRS)
require.NoError(t, err)
@@ -1382,7 +1416,15 @@
}`))
require.NoError(t, err)
- s := New(nil, mes, nil, mis, mcs, mts, nil, publicMatcher, nothingFlaky)
+ reviewSystems := []clstore.ReviewSystem{
+ {
+ ID: testCRS,
+ Store: mcs,
+ URLTemplate: "https://skia-review.googlesource.com/%s",
+ // Client and URLTemplate are unused here
+ },
+ }
+ s := New(nil, mes, nil, mis, reviewSystems, mts, nil, publicMatcher, nothingFlaky)
rv, err := s.GetDigestDetails(context.Background(), testWeWantDetailsAbout, digestWeWantDetailsAbout, testCLID, testCRS)
require.NoError(t, err)
diff --git a/golden/go/web/frontend/types.go b/golden/go/web/frontend/types.go
index 0ebe0c4..19b996c 100644
--- a/golden/go/web/frontend/types.go
+++ b/golden/go/web/frontend/types.go
@@ -99,6 +99,10 @@
// "issue" is the JSON field for backwards compatibility.
ChangeListID string `json:"issue"`
+ // CodeReviewSystem is the id of the crs that the ChangeListID belongs. If ChangeListID is set,
+ // CodeReviewSystem should be also.
+ CodeReviewSystem string `json:"crs"`
+
// ImageMatchingAlgorithm is the name of the non-exact image matching algorithm requesting the
// triage (see http://go/gold-non-exact-matching). If set, the algorithm name will be used as
// the author of the triage action.
@@ -214,11 +218,11 @@
// Commit represents a git Commit for use on the frontend.
type Commit struct {
// CommitTime is in seconds since the epoch
- CommitTime int64 `json:"commit_time"`
- Hash string `json:"hash"` // For CLs, this is the CL ID.
- Author string `json:"author"`
- Subject string `json:"message"`
- IsChangeList bool `json:"is_cl"`
+ CommitTime int64 `json:"commit_time"`
+ Hash string `json:"hash"` // For CLs, this is the CL ID.
+ Author string `json:"author"`
+ Subject string `json:"message"`
+ ChangeListURL string `json:"cl_url"`
}
// FromTilingCommit converts a tiling.Commit into a frontend.Commit.
diff --git a/golden/go/web/web.go b/golden/go/web/web.go
index 80f47f6..6fdced7 100644
--- a/golden/go/web/web.go
+++ b/golden/go/web/web.go
@@ -71,19 +71,18 @@
// HandlersConfig holds the environment needed by the various http handler functions.
type HandlersConfig struct {
- Baseliner baseline.BaselineFetcher
- ChangeListStore clstore.Store
- CodeReviewURLTemplate string
- DiffStore diff.DiffStore
- ExpectationsStore expectations.Store
- GCSClient storage.GCSClient
- IgnoreStore ignore.Store
- Indexer indexer.IndexSource
- SearchAPI search.SearchAPI
- StatusWatcher *status.StatusWatcher
- TileSource tilesource.TileSource
- TryJobStore tjstore.Store
- VCS vcsinfo.VCS
+ Baseliner baseline.BaselineFetcher
+ DiffStore diff.DiffStore
+ ExpectationsStore expectations.Store
+ GCSClient storage.GCSClient
+ IgnoreStore ignore.Store
+ Indexer indexer.IndexSource
+ ReviewSystems []clstore.ReviewSystem
+ SearchAPI search.SearchAPI
+ StatusWatcher *status.StatusWatcher
+ TileSource tilesource.TileSource
+ TryJobStore tjstore.Store
+ VCS vcsinfo.VCS
}
// Handlers represents all the handlers (e.g. JSON endpoints) of Gold.
@@ -105,17 +104,14 @@
if conf.Baseliner == nil {
return nil, skerr.Fmt("Baseliner cannot be nil")
}
- if conf.ChangeListStore == nil {
- return nil, skerr.Fmt("ChangeListStore cannot be nil")
+ if len(conf.ReviewSystems) == 0 {
+ return nil, skerr.Fmt("ReviewSystems cannot be empty")
}
if conf.GCSClient == nil {
return nil, skerr.Fmt("GCSClient cannot be nil")
}
if val == FullFrontEnd {
- if conf.CodeReviewURLTemplate == "" {
- return nil, skerr.Fmt("CodeReviewURLTemplate cannot be nil")
- }
if conf.DiffStore == nil {
return nil, skerr.Fmt("DiffStore cannot be nil")
}
@@ -360,20 +356,28 @@
so.OpenCLsOnly = true
}
- cls, total, err := wh.ChangeListStore.GetChangeLists(ctx, so)
- if err != nil {
- return nil, nil, skerr.Wrapf(err, "fetching ChangeLists from [%d:%d)", offset, offset+size)
- }
- crs := wh.ChangeListStore.System()
+ grandTotal := 0
var retCls []frontend.ChangeList
- for _, cl := range cls {
- retCls = append(retCls, frontend.ConvertChangeList(cl, crs, wh.CodeReviewURLTemplate))
+ for _, system := range wh.ReviewSystems {
+ cls, total, err := system.Store.GetChangeLists(ctx, so)
+ if err != nil {
+ return nil, nil, skerr.Wrapf(err, "fetching ChangeLists from [%d:%d)", offset, offset+size)
+ }
+
+ for _, cl := range cls {
+ retCls = append(retCls, frontend.ConvertChangeList(cl, system.ID, system.URLTemplate))
+ }
+ if grandTotal == clstore.CountMany || total == clstore.CountMany {
+ grandTotal = clstore.CountMany
+ } else {
+ grandTotal += total
+ }
}
pagination := &httputils.ResponsePagination{
Offset: offset,
Size: size,
- Total: total,
+ Total: grandTotal,
}
return retCls, pagination, nil
}
@@ -394,8 +398,18 @@
http.Error(w, "Must specify 'id' of ChangeList.", http.StatusBadRequest)
return
}
+ crs, ok := mux.Vars(r)["system"]
+ if !ok {
+ http.Error(w, "Must specify 'system' of ChangeList.", http.StatusBadRequest)
+ return
+ }
+ system, ok := wh.getCodeReviewSystem(crs)
+ if !ok {
+ http.Error(w, "Invalid Code Review System", http.StatusBadRequest)
+ return
+ }
- rv, err := wh.getCLSummary(r.Context(), clID)
+ rv, err := wh.getCLSummary(r.Context(), system, clID)
if err != nil {
httputils.ReportError(w, err, "could not retrieve data for the specified CL.", http.StatusInternalServerError)
return
@@ -413,19 +427,18 @@
// getCLSummary does a bulk of the work for ChangeListSummaryHandler, specifically
// fetching the ChangeList and PatchSets from clstore and any associated TryJobs from
// the tjstore.
-func (wh *Handlers) getCLSummary(ctx context.Context, clID string) (frontend.ChangeListSummary, error) {
- cl, err := wh.ChangeListStore.GetChangeList(ctx, clID)
+func (wh *Handlers) getCLSummary(ctx context.Context, system clstore.ReviewSystem, clID string) (frontend.ChangeListSummary, error) {
+ cl, err := system.Store.GetChangeList(ctx, clID)
if err != nil {
return frontend.ChangeListSummary{}, skerr.Wrapf(err, "getting CL %s", clID)
}
// We know xps is sorted by order, if it is non-nil
- xps, err := wh.ChangeListStore.GetPatchSets(ctx, clID)
+ xps, err := system.Store.GetPatchSets(ctx, clID)
if err != nil {
return frontend.ChangeListSummary{}, skerr.Wrapf(err, "getting PatchSets for CL %s", clID)
}
- crs := wh.ChangeListStore.System()
var patchsets []frontend.PatchSet
maxOrder := 0
@@ -436,7 +449,7 @@
}
psID := tjstore.CombinedPSID{
CL: clID,
- CRS: wh.ChangeListStore.System(),
+ CRS: system.ID,
PS: ps.SystemID,
}
xtj, err := wh.TryJobStore.GetTryJobs(ctx, psID)
@@ -457,7 +470,7 @@
}
return frontend.ChangeListSummary{
- CL: frontend.ConvertChangeList(cl, crs, wh.CodeReviewURLTemplate),
+ CL: frontend.ConvertChangeList(cl, system.ID, system.URLTemplate),
PatchSets: patchsets,
NumTotalPatchSets: maxOrder,
}, nil
@@ -605,7 +618,15 @@
return
}
clID := r.Form.Get("issue")
- crs := wh.ChangeListStore.System()
+ crs := r.Form.Get("crs")
+ if clID != "" {
+ if _, ok := wh.getCodeReviewSystem(crs); !ok {
+ http.Error(w, "Invalid Code Review System; did you include crs?", http.StatusBadRequest)
+ return
+ }
+ } else {
+ crs = ""
+ }
ret, err := wh.SearchAPI.GetDigestDetails(r.Context(), types.TestName(test), types.Digest(digest), clID, crs)
if err != nil {
@@ -637,7 +658,15 @@
return
}
clID := r.Form.Get("issue")
- crs := wh.ChangeListStore.System()
+ crs := r.Form.Get("crs")
+ if clID != "" {
+ if _, ok := wh.getCodeReviewSystem(crs); !ok {
+ http.Error(w, "Invalid Code Review System; did you include crs?", http.StatusBadRequest)
+ return
+ }
+ } else {
+ crs = ""
+ }
ret, err := wh.SearchAPI.DiffDigests(r.Context(), types.TestName(test), types.Digest(left), types.Digest(right), clID, crs)
if err != nil {
@@ -913,6 +942,19 @@
// triage processes the given TriageRequest.
func (wh *Handlers) triage(ctx context.Context, user string, req frontend.TriageRequest) error {
+ // TODO(kjlubick) remove the legacy check for "0" when the frontend no longer sends it.
+ if req.ChangeListID != "" && req.ChangeListID != "0" {
+ if req.CodeReviewSystem == "" {
+ // TODO(kjlubick) remove this default after the search page is converted to lit-html.
+ req.CodeReviewSystem = wh.ReviewSystems[0].ID
+ }
+ if _, ok := wh.getCodeReviewSystem(req.CodeReviewSystem); !ok {
+ return skerr.Fmt("Unknown Code Review System; did you remember to include crs?")
+ }
+ } else {
+ req.CodeReviewSystem = ""
+ }
+
// Build the expectations change request from the list of digests passed in.
tc := make([]expectations.Delta, 0, len(req.TestDigestStatus))
for test, digests := range req.TestDigestStatus {
@@ -939,7 +981,7 @@
expStore := wh.ExpectationsStore
// TODO(kjlubick) remove the legacy check here after the frontend bakes in.
if req.ChangeListID != "" && req.ChangeListID != "0" {
- expStore = wh.ExpectationsStore.ForChangeList(req.ChangeListID, wh.ChangeListStore.System())
+ expStore = wh.ExpectationsStore.ForChangeList(req.ChangeListID, req.CodeReviewSystem)
}
// If set, use the image matching algorithm's name as the author of this change.
@@ -993,8 +1035,8 @@
}
// TODO(kjlubick): Check if we need to sort these
- // // Sort the digests so they are displayed with untriaged last, which means
- // // they will be displayed 'on top', because in SVG document order is z-order.
+ // Sort the digests so they are displayed with untriaged last, which means
+ // they will be displayed 'on top', because in SVG document order is z-order.
digests := types.DigestSlice{}
for _, digest := range searchResponse.Results {
@@ -1128,10 +1170,19 @@
return
}
- changeList := q.Get("issue")
+ clID := q.Get("issue")
+ crs := q.Get("crs")
+ if clID != "" {
+ if _, ok := wh.getCodeReviewSystem(crs); !ok {
+ http.Error(w, "Invalid Code Review System; did you include crs?", http.StatusBadRequest)
+ return
+ }
+ } else {
+ crs = ""
+ }
details := q.Get("details") == "true"
- logEntries, total, err := wh.getTriageLog(r.Context(), changeList, offset, size, details)
+ logEntries, total, err := wh.getTriageLog(r.Context(), crs, clID, offset, size, details)
if err != nil {
httputils.ReportError(w, err, "Unable to retrieve triage logs", http.StatusInternalServerError)
@@ -1148,11 +1199,11 @@
}
// getTriageLog does the actual work of the TriageLogHandler, but is easier to test.
-func (wh *Handlers) getTriageLog(ctx context.Context, changeListID string, offset, size int, withDetails bool) ([]frontend.TriageLogEntry, int, error) {
+func (wh *Handlers) getTriageLog(ctx context.Context, crs, changeListID string, offset, size int, withDetails bool) ([]frontend.TriageLogEntry, int, error) {
expStore := wh.ExpectationsStore
// TODO(kjlubick) remove this legacy handler
if changeListID != "" && changeListID != "0" {
- expStore = wh.ExpectationsStore.ForChangeList(changeListID, wh.ChangeListStore.System())
+ expStore = wh.ExpectationsStore.ForChangeList(changeListID, crs)
}
entries, total, err := expStore.QueryLog(ctx, offset, size, withDetails)
if err != nil {
@@ -1207,8 +1258,21 @@
return
}
clID := r.Form.Get("changelist_id")
+ crs := r.Form.Get("crs")
if clID != "" {
- crs := wh.ChangeListStore.System()
+ if crs == "" {
+ // TODO(kjlubick) remove this default after the search page is converted to lit-html.
+ crs = wh.ReviewSystems[0].ID
+ }
+ if _, ok := wh.getCodeReviewSystem(crs); !ok {
+ http.Error(w, "Invalid Code Review System; did you include crs?", http.StatusBadRequest)
+ return
+ }
+ } else {
+ crs = ""
+ }
+
+ if clID != "" {
clIdx := wh.Indexer.GetIndexForCL(crs, clID)
if clIdx != nil {
sendJSONResponse(w, clIdx.ParamSet)
@@ -1289,10 +1353,25 @@
metrics2.GetCounter("gold_baselinehandler_route_new").Inc(1)
}
- clID := r.URL.Query().Get("issue")
- issueOnly := r.URL.Query().Get("issueOnly") == "true"
+ q := r.URL.Query()
+ clID := q.Get("issue")
+ issueOnly := q.Get("issueOnly") == "true"
+ crs := q.Get("crs")
- bl, err := wh.Baseliner.FetchBaseline(r.Context(), clID, wh.ChangeListStore.System(), issueOnly)
+ if clID != "" {
+ if crs == "" {
+ // TODO(kjlubick) remove this default after the search page is converted to lit-html.
+ crs = wh.ReviewSystems[0].ID
+ }
+ if _, ok := wh.getCodeReviewSystem(crs); !ok {
+ http.Error(w, "Invalid CRS provided.", http.StatusBadRequest)
+ return
+ }
+ } else {
+ crs = ""
+ }
+
+ bl, err := wh.Baseliner.FetchBaseline(r.Context(), clID, crs, issueOnly)
if err != nil {
httputils.ReportError(w, err, "Fetching baselines failed.", http.StatusInternalServerError)
return
@@ -1508,13 +1587,18 @@
http.Error(w, "Must specify 'id' of ChangeList.", http.StatusBadRequest)
return
}
+ system, ok := wh.getCodeReviewSystem(crs)
+ if !ok {
+ http.Error(w, "Invalid Code Review System", http.StatusBadRequest)
+ return
+ }
- baseURL := fmt.Sprintf("/search?issue=%s", clID)
+ baseURL := fmt.Sprintf("/search?issue=%s&crs=%s", clID, system.ID)
- clIdx := wh.Indexer.GetIndexForCL(crs, clID)
+ clIdx := wh.Indexer.GetIndexForCL(system.ID, clID)
if clIdx == nil {
// Not cached, so we can't cheaply determine the corpus to include
- if _, err := wh.ChangeListStore.GetChangeList(r.Context(), clID); err != nil {
+ if _, err := system.Store.GetChangeList(r.Context(), clID); err != nil {
http.NotFound(w, r)
return
}
@@ -1551,3 +1635,15 @@
}
return login.LoggedInAs(r)
}
+
+func (wh *Handlers) getCodeReviewSystem(crs string) (clstore.ReviewSystem, bool) {
+ var system clstore.ReviewSystem
+ found := false
+ for _, rs := range wh.ReviewSystems {
+ if rs.ID == crs {
+ system = rs
+ found = true
+ }
+ }
+ return system, found
+}
diff --git a/golden/go/web/web_test.go b/golden/go/web/web_test.go
index b9cddfb..c99c1e0 100644
--- a/golden/go/web/web_test.go
+++ b/golden/go/web/web_test.go
@@ -16,11 +16,11 @@
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
- "go.skia.org/infra/go/paramtools"
"golang.org/x/time/rate"
"go.skia.org/infra/go/httputils"
"go.skia.org/infra/go/metrics2"
+ "go.skia.org/infra/go/paramtools"
"go.skia.org/infra/go/testutils"
"go.skia.org/infra/go/testutils/unittest"
"go.skia.org/infra/golden/go/baseline"
@@ -28,6 +28,7 @@
"go.skia.org/infra/golden/go/clstore"
mock_clstore "go.skia.org/infra/golden/go/clstore/mocks"
"go.skia.org/infra/golden/go/code_review"
+ mock_crs "go.skia.org/infra/golden/go/code_review/mocks"
ci "go.skia.org/infra/golden/go/continuous_integration"
"go.skia.org/infra/golden/go/digest_counter"
"go.skia.org/infra/golden/go/expectations"
@@ -78,9 +79,15 @@
unittest.SmallTest(t)
hc := HandlersConfig{
- GCSClient: &mocks.GCSClient{},
- Baseliner: &mocks.BaselineFetcher{},
- ChangeListStore: &mock_clstore.Store{},
+ GCSClient: &mocks.GCSClient{},
+ Baseliner: &mocks.BaselineFetcher{},
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: "whatever",
+ Store: &mock_clstore.Store{},
+ Client: &mock_crs.Client{},
+ },
+ },
}
_, err := NewHandlers(hc, BaselineSubset)
require.NoError(t, err)
@@ -97,12 +104,12 @@
assert.Contains(t, err.Error(), "cannot be nil")
hc = HandlersConfig{
- GCSClient: &mocks.GCSClient{},
- ChangeListStore: &mock_clstore.Store{},
+ Baseliner: &mocks.BaselineFetcher{},
+ GCSClient: &mocks.GCSClient{},
}
_, err = NewHandlers(hc, BaselineSubset)
require.Error(t, err)
- assert.Contains(t, err.Error(), "cannot be nil")
+ assert.Contains(t, err.Error(), "cannot be empty")
}
// TestNewHandlers_FullFront_EndMissingPieces_Failure makes sure that if we omit values from
@@ -118,9 +125,15 @@
assert.Contains(t, err.Error(), "cannot be nil")
hc = HandlersConfig{
- GCSClient: &mocks.GCSClient{},
- Baseliner: &mocks.BaselineFetcher{},
- ChangeListStore: &mock_clstore.Store{},
+ GCSClient: &mocks.GCSClient{},
+ Baseliner: &mocks.BaselineFetcher{},
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: "whatever",
+ Store: &mock_clstore.Store{},
+ Client: &mock_crs.Client{},
+ },
+ },
}
_, err = NewHandlers(hc, FullFrontEnd)
require.Error(t, err)
@@ -315,12 +328,16 @@
StartIdx: 0,
Limit: 50,
}).Return(makeCodeReviewCLs(), len(makeCodeReviewCLs()), nil)
- mcls.On("System").Return("gerrit")
wh := Handlers{
HandlersConfig: HandlersConfig{
- CodeReviewURLTemplate: "example.com/cl/%s#templates",
- ChangeListStore: mcls,
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: "gerrit",
+ Store: mcls,
+ URLTemplate: "example.com/cl/%s#templates",
+ },
+ },
},
}
@@ -352,12 +369,16 @@
Limit: 30,
OpenCLsOnly: true,
}).Return(makeCodeReviewCLs(), 3, nil)
- mcls.On("System").Return("gerrit")
wh := Handlers{
HandlersConfig: HandlersConfig{
- CodeReviewURLTemplate: "example.com/cl/%s#templates",
- ChangeListStore: mcls,
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: "gerrit",
+ Store: mcls,
+ URLTemplate: "example.com/cl/%s#templates",
+ },
+ },
},
}
@@ -440,6 +461,7 @@
unittest.SmallTest(t)
const expectedCLID = "1002"
+ const gerritCRS = "gerrit"
mcls := &mock_clstore.Store{}
mtjs := &mock_tjstore.Store{}
@@ -450,7 +472,7 @@
psID := tjstore.CombinedPSID{
CL: expectedCLID,
- CRS: "gerrit",
+ CRS: gerritCRS,
PS: "ps-1",
}
tj1 := []ci.TryJob{
@@ -465,7 +487,7 @@
psID = tjstore.CombinedPSID{
CL: expectedCLID,
- CRS: "gerrit",
+ CRS: gerritCRS,
PS: "ps-4",
}
tj2 := []ci.TryJob{
@@ -484,15 +506,20 @@
}
mtjs.On("GetTryJobs", testutils.AnyContext, psID).Return(tj2, nil)
+ gerritSystem := clstore.ReviewSystem{
+ ID: gerritCRS,
+ Store: mcls,
+ URLTemplate: "example.com/cl/%s#templates",
+ }
+
wh := Handlers{
HandlersConfig: HandlersConfig{
- CodeReviewURLTemplate: "example.com/cl/%s#templates",
- ChangeListStore: mcls,
- TryJobStore: mtjs,
+ ReviewSystems: []clstore.ReviewSystem{gerritSystem},
+ TryJobStore: mtjs,
},
}
- cl, err := wh.getCLSummary(context.Background(), expectedCLID)
+ cl, err := wh.getCLSummary(context.Background(), gerritSystem, expectedCLID)
assert.NoError(t, err)
assert.Equal(t, frontend.ChangeListSummary{
CL: makeWebCLs()[0], // matches expectedCLID
@@ -578,7 +605,6 @@
}
tr := frontend.TriageRequest{
- ChangeListID: "",
TestDigestStatus: map[types.TestName]map[types.Digest]expectations.Label{
bug_revert.TestOne: {
bug_revert.BravoUntriagedDigest: expectations.Negative,
@@ -614,7 +640,6 @@
}
tr := frontend.TriageRequest{
- ChangeListID: "",
TestDigestStatus: map[types.TestName]map[types.Digest]expectations.Label{
bug_revert.TestOne: {
bug_revert.BravoUntriagedDigest: expectations.Negative,
@@ -634,16 +659,13 @@
mes := &mock_expectations.Store{}
clExp := &mock_expectations.Store{}
- mcs := &mock_clstore.Store{}
- defer mes.AssertExpectations(t)
defer clExp.AssertExpectations(t)
- defer mcs.AssertExpectations(t)
const clID = "12345"
- const crs = "github"
+ const githubCRS = "github"
const user = "user@example.com"
- mes.On("ForChangeList", clID, crs).Return(clExp)
+ mes.On("ForChangeList", clID, githubCRS).Return(clExp)
clExp.On("AddChange", testutils.AnyContext, []expectations.Delta{
{
@@ -653,17 +675,21 @@
},
}, user).Return(nil)
- mcs.On("System").Return(crs)
-
wh := Handlers{
HandlersConfig: HandlersConfig{
ExpectationsStore: mes,
- ChangeListStore: mcs,
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: githubCRS,
+ // The rest is unused here
+ },
+ },
},
}
tr := frontend.TriageRequest{
- ChangeListID: clID,
+ CodeReviewSystem: githubCRS,
+ ChangeListID: clID,
TestDigestStatus: map[types.TestName]map[types.Digest]expectations.Label{
bug_revert.TestOne: {
bug_revert.BravoUntriagedDigest: expectations.Negative,
@@ -680,17 +706,15 @@
mes := &mock_expectations.Store{}
clExp := &mock_expectations.Store{}
- mcs := &mock_clstore.Store{}
defer mes.AssertExpectations(t)
defer clExp.AssertExpectations(t)
- defer mcs.AssertExpectations(t)
const clID = "12345"
- const crs = "github"
+ const githubCRS = "github"
const user = "user@example.com"
const algorithmName = "fuzzy"
- mes.On("ForChangeList", clID, crs).Return(clExp)
+ mes.On("ForChangeList", clID, githubCRS).Return(clExp)
clExp.On("AddChange", testutils.AnyContext, []expectations.Delta{
{
@@ -700,17 +724,20 @@
},
}, algorithmName).Return(nil)
- mcs.On("System").Return(crs)
-
wh := Handlers{
HandlersConfig: HandlersConfig{
ExpectationsStore: mes,
- ChangeListStore: mcs,
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: githubCRS,
+ },
+ },
},
}
tr := frontend.TriageRequest{
- ChangeListID: clID,
+ CodeReviewSystem: githubCRS,
+ ChangeListID: clID,
TestDigestStatus: map[types.TestName]map[types.Digest]expectations.Label{
bug_revert.TestOne: {
bug_revert.BravoUntriagedDigest: expectations.Negative,
@@ -766,7 +793,6 @@
}
tr := frontend.TriageRequest{
- ChangeListID: "",
TestDigestStatus: map[types.TestName]map[types.Digest]expectations.Label{
bug_revert.TestOne: {
bug_revert.AlfaPositiveDigest: expectations.Untriaged,
@@ -877,7 +903,7 @@
},
}, offset+2, nil)
- tle, n, err := wh.getTriageLog(context.Background(), masterBranch, offset, size, false)
+ tle, n, err := wh.getTriageLog(context.Background(), masterBranch, masterBranch, offset, size, false)
assert.NoError(t, err)
assert.Equal(t, offset+2, n)
assert.Len(t, tle, 2)
@@ -1412,25 +1438,28 @@
func TestBaselineHandler_Success(t *testing.T) {
unittest.SmallTest(t)
+ const gerritCRS = "gerrit"
mbf := &mocks.BaselineFetcher{}
- mcls := &mock_clstore.Store{}
- mcls.On("System").Return("gerrit")
wh := Handlers{
HandlersConfig: HandlersConfig{
- Baseliner: mbf,
- ChangeListStore: mcls,
+ Baseliner: mbf,
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ },
+ },
},
}
w := httptest.NewRecorder()
r := httptest.NewRequest(http.MethodGet, requestURL, nil)
// Prepare a fake response from the BaselineFetcher and the handler's expected JSON response.
- bl := &baseline.Baseline{ChangeListID: "", MD5: "fakehash", CodeReviewSystem: "gerrit"}
+ bl := &baseline.Baseline{ChangeListID: "", MD5: "fakehash", CodeReviewSystem: gerritCRS}
expectedJSONResponse := `{"md5":"fakehash","master_str":null,"crs":"gerrit"}`
// FetchBaseline should be called as per the request parameters.
- mbf.On("FetchBaseline", testutils.AnyContext, "" /* =clID */, "gerrit", false /* =issueOnly */).Return(bl, nil)
+ mbf.On("FetchBaseline", testutils.AnyContext, "" /* =clID */, "", false /* =issueOnly */).Return(bl, nil)
defer mbf.AssertExpectations(t) // Assert that the method above was called exactly as expected.
// We'll use the counters to assert that the right route was called.
@@ -1451,14 +1480,17 @@
func TestBaselineHandler_IssueSet_Success(t *testing.T) {
unittest.SmallTest(t)
+ const gerritCRS = "gerrit"
mbf := &mocks.BaselineFetcher{}
- mcls := &mock_clstore.Store{}
- mcls.On("System").Return("gerrit")
wh := Handlers{
HandlersConfig: HandlersConfig{
- Baseliner: mbf,
- ChangeListStore: mcls,
+ Baseliner: mbf,
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ },
+ },
},
}
w := httptest.NewRecorder()
@@ -1467,11 +1499,11 @@
r := httptest.NewRequest(http.MethodGet, requestURL+"?issue=123456", nil)
// Prepare a fake response from the BaselineFetcher and the handler's expected JSON response.
- bl := &baseline.Baseline{ChangeListID: "", MD5: "fakehash", CodeReviewSystem: "gerrit"}
+ bl := &baseline.Baseline{ChangeListID: "", MD5: "fakehash", CodeReviewSystem: gerritCRS}
expectedJSONResponse := `{"md5":"fakehash","master_str":null,"crs":"gerrit"}`
// FetchBaseline should be called as per the request parameters.
- mbf.On("FetchBaseline", testutils.AnyContext, "123456" /* =clID */, "gerrit", false /* =issueOnly */).Return(bl, nil)
+ mbf.On("FetchBaseline", testutils.AnyContext, "123456" /* =clID */, gerritCRS, false /* =issueOnly */).Return(bl, nil)
defer mbf.AssertExpectations(t) // Assert that the method above was called exactly as expected.
// We'll use the counters to assert that the right route was called.
@@ -1492,14 +1524,17 @@
func TestBaselineHandler_IssueSet_IssueOnly_Success(t *testing.T) {
unittest.SmallTest(t)
+ const gerritCRS = "gerrit"
mbf := &mocks.BaselineFetcher{}
- mcls := &mock_clstore.Store{}
- mcls.On("System").Return("gerrit")
wh := Handlers{
HandlersConfig: HandlersConfig{
- Baseliner: mbf,
- ChangeListStore: mcls,
+ Baseliner: mbf,
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ },
+ },
},
}
w := httptest.NewRecorder()
@@ -1508,11 +1543,11 @@
r := httptest.NewRequest(http.MethodGet, requestURL+"?issue=123456&issueOnly=true", nil)
// Prepare a fake response from the BaselineFetcher and the handler's expected JSON response.
- bl := &baseline.Baseline{ChangeListID: "", MD5: "fakehash", CodeReviewSystem: "gerrit"}
+ bl := &baseline.Baseline{ChangeListID: "", MD5: "fakehash", CodeReviewSystem: gerritCRS}
expectedJSONResponse := `{"md5":"fakehash","master_str":null,"crs":"gerrit"}`
// FetchBaseline should be called as per the request parameters.
- mbf.On("FetchBaseline", testutils.AnyContext, "123456" /* =clID */, "gerrit", true /* =issueOnly */).Return(bl, nil)
+ mbf.On("FetchBaseline", testutils.AnyContext, "123456" /* =clID */, gerritCRS, true /* =issueOnly */).Return(bl, nil)
defer mbf.AssertExpectations(t) // Assert that the method above was called exactly as expected.
// We'll use the counters to assert that the right route was called.
@@ -1533,16 +1568,18 @@
func TestBaselineHandler_BaselineFetcherError_InternalServerError(t *testing.T) {
unittest.SmallTest(t)
+ const gerritCRS = "gerrit"
mbf := &mocks.BaselineFetcher{}
mbf.On("FetchBaseline", testutils.AnyContext, mock.Anything, mock.Anything, mock.Anything).Return(nil, errors.New("oops"))
- mcls := &mock_clstore.Store{}
- mcls.On("System").Return("gerrit")
-
wh := Handlers{
HandlersConfig: HandlersConfig{
- Baseliner: mbf,
- ChangeListStore: mcls,
+ Baseliner: mbf,
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ },
+ },
},
}
w := httptest.NewRecorder()
@@ -1568,14 +1605,17 @@
func TestBaselineHandler_CommitHashSet_IgnoresCommitHash_Success(t *testing.T) {
unittest.SmallTest(t)
+ const gerritCRS = "gerrit"
mbf := &mocks.BaselineFetcher{}
- mcls := &mock_clstore.Store{}
- mcls.On("System").Return("gerrit")
wh := Handlers{
HandlersConfig: HandlersConfig{
- Baseliner: mbf,
- ChangeListStore: mcls,
+ Baseliner: mbf,
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ },
+ },
},
}
w := httptest.NewRecorder()
@@ -1585,11 +1625,11 @@
r = mux.SetURLVars(r, map[string]string{"commit_hash": "09e87c3d93e2bb188a8dae01b7f8b9ffb2ebcad1"})
// Prepare a fake response from the BaselineFetcher and the handler's expected JSON response.
- bl := &baseline.Baseline{ChangeListID: "", MD5: "fakehash", CodeReviewSystem: "gerrit"}
+ bl := &baseline.Baseline{ChangeListID: "", MD5: "fakehash", CodeReviewSystem: gerritCRS}
expectedJSONResponse := `{"md5":"fakehash","master_str":null,"crs":"gerrit"}`
// Note that the {commit_hash} doesn't appear anywhere in the FetchBaseline call.
- mbf.On("FetchBaseline", testutils.AnyContext, "" /* =clID */, "gerrit", false /* =issueOnly */).Return(bl, nil)
+ mbf.On("FetchBaseline", testutils.AnyContext, "" /* =clID */, "", false /* =issueOnly */).Return(bl, nil)
defer mbf.AssertExpectations(t) // Assert that the method above was called exactly as expected.
// We'll use the counters to assert that the right route was called.
@@ -1612,14 +1652,17 @@
func TestBaselineHandler_CommitHashSet_IssueSet_IgnoresCommitHash_Success(t *testing.T) {
unittest.SmallTest(t)
+ const gerritCRS = "gerrit"
mbf := &mocks.BaselineFetcher{}
- mcls := &mock_clstore.Store{}
- mcls.On("System").Return("gerrit")
wh := Handlers{
HandlersConfig: HandlersConfig{
- Baseliner: mbf,
- ChangeListStore: mcls,
+ Baseliner: mbf,
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ },
+ },
},
}
w := httptest.NewRecorder()
@@ -1631,11 +1674,11 @@
r = mux.SetURLVars(r, map[string]string{"commit_hash": "09e87c3d93e2bb188a8dae01b7f8b9ffb2ebcad1"})
// Prepare a fake response from the BaselineFetcher and the handler's expected JSON response.
- bl := &baseline.Baseline{ChangeListID: "", MD5: "fakehash", CodeReviewSystem: "gerrit"}
+ bl := &baseline.Baseline{ChangeListID: "", MD5: "fakehash", CodeReviewSystem: gerritCRS}
expectedJSONResponse := `{"md5":"fakehash","master_str":null,"crs":"gerrit"}`
// Note that the {commit_hash} doesn't appear anywhere in the FetchBaseline call.
- mbf.On("FetchBaseline", testutils.AnyContext, "123456" /* =clID */, "gerrit", false /* =issueOnly */).Return(bl, nil)
+ mbf.On("FetchBaseline", testutils.AnyContext, "123456" /* =clID */, gerritCRS, false /* =issueOnly */).Return(bl, nil)
defer mbf.AssertExpectations(t) // Assert that the method above was called exactly as expected.
// We'll use the counters to assert that the right route was called.
@@ -1658,14 +1701,17 @@
func TestBaselineHandler_CommitHashSet_IssueSet_IssueOnly_IgnoresCommitHash_Success(t *testing.T) {
unittest.SmallTest(t)
+ const gerritCRS = "gerrit"
mbf := &mocks.BaselineFetcher{}
- mcls := &mock_clstore.Store{}
- mcls.On("System").Return("gerrit")
wh := Handlers{
HandlersConfig: HandlersConfig{
- Baseliner: mbf,
- ChangeListStore: mcls,
+ Baseliner: mbf,
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ },
+ },
},
}
w := httptest.NewRecorder()
@@ -1677,11 +1723,11 @@
r = mux.SetURLVars(r, map[string]string{"commit_hash": "09e87c3d93e2bb188a8dae01b7f8b9ffb2ebcad1"})
// Prepare a fake response from the BaselineFetcher and the handler's expected JSON response.
- bl := &baseline.Baseline{ChangeListID: "", MD5: "fakehash", CodeReviewSystem: "gerrit"}
+ bl := &baseline.Baseline{ChangeListID: "", MD5: "fakehash", CodeReviewSystem: gerritCRS}
expectedJSONResponse := `{"md5":"fakehash","master_str":null,"crs":"gerrit"}`
// Note that the {commit_hash} doesn't appear anywhere in the FetchBaseline call.
- mbf.On("FetchBaseline", testutils.AnyContext, "123456" /* =clID */, "gerrit", true /* =issueOnly */).Return(bl, nil)
+ mbf.On("FetchBaseline", testutils.AnyContext, "123456" /* =clID */, gerritCRS, true /* =issueOnly */).Return(bl, nil)
defer mbf.AssertExpectations(t) // Assert that the method above was called exactly as expected.
// We'll use the counters to assert that the right route was called.
@@ -1881,11 +1927,10 @@
func TestParamsHandler_ChangeListIndex_Success(t *testing.T) {
unittest.SmallTest(t)
- mockCLStore := &mock_clstore.Store{}
mockIndexSource := &mock_indexer.IndexSource{}
defer mockIndexSource.AssertExpectations(t) // want to make sure fallback happened
- const crs = "gerrit"
+ const gerritCRS = "gerrit"
const clID = "1234"
clIdx := indexer.ChangeListIndex{
@@ -1895,18 +1940,21 @@
"os": []string{"Android XYZ"},
},
}
- mockIndexSource.On("GetIndexForCL", crs, clID).Return(&clIdx)
- mockCLStore.On("System").Return(crs)
+ mockIndexSource.On("GetIndexForCL", gerritCRS, clID).Return(&clIdx)
wh := Handlers{
HandlersConfig: HandlersConfig{
- Indexer: mockIndexSource,
- ChangeListStore: mockCLStore,
+ Indexer: mockIndexSource,
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ },
+ },
},
anonymousCheapQuota: rate.NewLimiter(rate.Inf, 1),
}
w := httptest.NewRecorder()
- r := httptest.NewRequest(http.MethodGet, "/json/paramset?changelist_id=1234", nil)
+ r := httptest.NewRequest(http.MethodGet, "/json/paramset?changelist_id=1234&crs=gerrit", nil)
wh.ParamsHandler(w, r)
const expectedResponse = `{"name":["alpha_test","beta_test","gamma_test"],"os":["Android XYZ"],"source_type":["first_corpus","second_corpus"]}`
assertJSONResponseWas(t, http.StatusOK, expectedResponse, w)
@@ -1915,16 +1963,15 @@
func TestParamsHandler_NoChangeListIndex_FallBackToMasterBranch(t *testing.T) {
unittest.SmallTest(t)
- mockCLStore := &mock_clstore.Store{}
mockIndexSearcher := &mock_indexer.IndexSearcher{}
mockIndexSource := &mock_indexer.IndexSource{}
defer mockIndexSource.AssertExpectations(t) // want to make sure fallback happened
- const crs = "gerrit"
+ const gerritCRS = "gerrit"
const clID = "1234"
mockIndexSource.On("GetIndex").Return(mockIndexSearcher)
- mockIndexSource.On("GetIndexForCL", crs, clID).Return(nil)
+ mockIndexSource.On("GetIndexForCL", gerritCRS, clID).Return(nil)
cpxTile := tiling.NewComplexTile(&tiling.Tile{
ParamSet: paramtools.ParamSet{
@@ -1937,17 +1984,19 @@
mockIndexSearcher.On("Tile").Return(cpxTile)
- mockCLStore.On("System").Return(crs)
-
wh := Handlers{
HandlersConfig: HandlersConfig{
- Indexer: mockIndexSource,
- ChangeListStore: mockCLStore,
+ Indexer: mockIndexSource,
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ },
+ },
},
anonymousCheapQuota: rate.NewLimiter(rate.Inf, 1),
}
w := httptest.NewRecorder()
- r := httptest.NewRequest(http.MethodGet, "/json/paramset?changelist_id=1234", nil)
+ r := httptest.NewRequest(http.MethodGet, "/json/paramset?changelist_id=1234&crs=gerrit", nil)
wh.ParamsHandler(w, r)
const expectedResponse = `{"name":["alpha_test","beta_test","gamma_test"],"os":["Android XYZ"],"source_type":["first_corpus","second_corpus"]}`
assertJSONResponseWas(t, http.StatusOK, expectedResponse, w)
@@ -1959,16 +2008,16 @@
mockSearchAPI := &mock_search.SearchAPI{}
mockIndexSource := &mock_indexer.IndexSource{}
- const crs = "gerrit"
+ const gerritCRS = "gerrit"
const clID = "1234"
combinedID := tjstore.CombinedPSID{
- CRS: crs,
+ CRS: gerritCRS,
CL: clID,
PS: "some patchset",
}
- mockIndexSource.On("GetIndexForCL", crs, clID).Return(&indexer.ChangeListIndex{
+ mockIndexSource.On("GetIndexForCL", gerritCRS, clID).Return(&indexer.ChangeListIndex{
LatestPatchSet: combinedID,
// Other fields should be ignored
})
@@ -1981,19 +2030,24 @@
HandlersConfig: HandlersConfig{
Indexer: mockIndexSource,
SearchAPI: mockSearchAPI,
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ },
+ },
},
anonymousCheapQuota: rate.NewLimiter(rate.Inf, 1),
}
w := httptest.NewRecorder()
r := httptest.NewRequest(http.MethodGet, "/cl/gerrit/1234", nil)
r = mux.SetURLVars(r, map[string]string{
- "system": crs,
+ "system": gerritCRS,
"id": clID,
})
wh.ChangeListSearchRedirect(w, r)
assert.Equal(t, http.StatusTemporaryRedirect, w.Code)
headers := w.Header()
- assert.Equal(t, []string{"/search?issue=1234&query=source_type%3Done_corpus"}, headers["Location"])
+ assert.Equal(t, []string{"/search?issue=1234&crs=gerrit&query=source_type%3Done_corpus"}, headers["Location"])
}
func TestChangeListSearchRedirect_ErrorGettingUntriagedDigests_RedirectsWithoutCorpus(t *testing.T) {
@@ -2003,16 +2057,16 @@
mockIndexSource := &mock_indexer.IndexSource{}
defer mockSearchAPI.AssertExpectations(t) // make sure the error was returned
- const crs = "gerrit"
+ const gerritCRS = "gerrit"
const clID = "1234"
combinedID := tjstore.CombinedPSID{
- CRS: crs,
+ CRS: gerritCRS,
CL: clID,
PS: "some patchset",
}
- mockIndexSource.On("GetIndexForCL", crs, clID).Return(&indexer.ChangeListIndex{
+ mockIndexSource.On("GetIndexForCL", gerritCRS, clID).Return(&indexer.ChangeListIndex{
LatestPatchSet: combinedID,
// Other fields should be ignored
})
@@ -2023,19 +2077,24 @@
HandlersConfig: HandlersConfig{
Indexer: mockIndexSource,
SearchAPI: mockSearchAPI,
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ },
+ },
},
anonymousCheapQuota: rate.NewLimiter(rate.Inf, 1),
}
w := httptest.NewRecorder()
r := httptest.NewRequest(http.MethodGet, "/cl/gerrit/1234", nil)
r = mux.SetURLVars(r, map[string]string{
- "system": crs,
+ "system": gerritCRS,
"id": clID,
})
wh.ChangeListSearchRedirect(w, r)
assert.Equal(t, http.StatusTemporaryRedirect, w.Code)
headers := w.Header()
- assert.Equal(t, []string{"/search?issue=1234"}, headers["Location"])
+ assert.Equal(t, []string{"/search?issue=1234&crs=gerrit"}, headers["Location"])
}
func TestChangeListSearchRedirect_CLExistsNotIndexed_RedirectsWithoutCorpus(t *testing.T) {
@@ -2044,31 +2103,36 @@
mockIndexSource := &mock_indexer.IndexSource{}
mockCLStore := &mock_clstore.Store{}
- const crs = "gerrit"
+ const gerritCRS = "gerrit"
const clID = "1234"
- mockIndexSource.On("GetIndexForCL", crs, clID).Return(nil)
+ mockIndexSource.On("GetIndexForCL", gerritCRS, clID).Return(nil)
// all this cares about is a non-error return code
mockCLStore.On("GetChangeList", testutils.AnyContext, clID).Return(code_review.ChangeList{}, nil)
wh := Handlers{
HandlersConfig: HandlersConfig{
- Indexer: mockIndexSource,
- ChangeListStore: mockCLStore,
+ Indexer: mockIndexSource,
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ Store: mockCLStore,
+ },
+ },
},
anonymousCheapQuota: rate.NewLimiter(rate.Inf, 1),
}
w := httptest.NewRecorder()
r := httptest.NewRequest(http.MethodGet, "/cl/gerrit/1234", nil)
r = mux.SetURLVars(r, map[string]string{
- "system": crs,
+ "system": gerritCRS,
"id": clID,
})
wh.ChangeListSearchRedirect(w, r)
assert.Equal(t, http.StatusTemporaryRedirect, w.Code)
headers := w.Header()
- assert.Equal(t, []string{"/search?issue=1234"}, headers["Location"])
+ assert.Equal(t, []string{"/search?issue=1234&crs=gerrit"}, headers["Location"])
}
func TestChangeListSearchRedirect_CLDoesNotExist_404Error(t *testing.T) {
@@ -2077,25 +2141,30 @@
mockIndexSource := &mock_indexer.IndexSource{}
mockCLStore := &mock_clstore.Store{}
- const crs = "gerrit"
+ const gerritCRS = "gerrit"
const clID = "1234"
- mockIndexSource.On("GetIndexForCL", crs, clID).Return(nil)
+ mockIndexSource.On("GetIndexForCL", gerritCRS, clID).Return(nil)
// all this cares about is a non-error return code
mockCLStore.On("GetChangeList", testutils.AnyContext, clID).Return(code_review.ChangeList{}, code_review.ErrNotFound)
wh := Handlers{
HandlersConfig: HandlersConfig{
- Indexer: mockIndexSource,
- ChangeListStore: mockCLStore,
+ Indexer: mockIndexSource,
+ ReviewSystems: []clstore.ReviewSystem{
+ {
+ ID: gerritCRS,
+ Store: mockCLStore,
+ },
+ },
},
anonymousCheapQuota: rate.NewLimiter(rate.Inf, 1),
}
w := httptest.NewRecorder()
r := httptest.NewRequest(http.MethodGet, "/cl/gerrit/1234", nil)
r = mux.SetURLVars(r, map[string]string{
- "system": crs,
+ "system": gerritCRS,
"id": clID,
})
wh.ChangeListSearchRedirect(w, r)
diff --git a/golden/k8s-instances/chrome-gpu/chrome-gpu-ingestion-bt.json5 b/golden/k8s-instances/chrome-gpu/chrome-gpu-ingestion-bt.json5
index caa5d12..2a5972b 100644
--- a/golden/k8s-instances/chrome-gpu/chrome-gpu-ingestion-bt.json5
+++ b/golden/k8s-instances/chrome-gpu/chrome-gpu-ingestion-bt.json5
@@ -36,7 +36,7 @@
FirestoreProjectID: "skia-firestore",
FirestoreNamespace: "chrome-gpu",
- CodeReviewSystem: "gerrit",
+ CodeReviewSystems: "gerrit",
GerritURL: "https://chromium-review.googlesource.com",
ContinuousIntegrationSystems: "buildbucket",
diff --git a/golden/k8s-instances/chrome-gpu/chrome-gpu-skiacorrectness.json5 b/golden/k8s-instances/chrome-gpu/chrome-gpu-skiacorrectness.json5
index 2d5e964..7350f89 100644
--- a/golden/k8s-instances/chrome-gpu/chrome-gpu-skiacorrectness.json5
+++ b/golden/k8s-instances/chrome-gpu/chrome-gpu-skiacorrectness.json5
@@ -11,17 +11,14 @@
If all the trybots passed and you don't expect your CL to have any effect on browser UI, you can likely ignore this message.\n\
See the FAQ for more information: https://docs.google.com/document/d/1BnwcxzhT8FFvY3YF-6BT4Mqgrb9U40t0HMfEVSSEpNs/edit?usp=sharing\n",
client_secret_file: "/etc/skia.org/login.json",
- crs_url_template: "https://chromium-review.googlesource.com/%s",
diff_server_grpc: "gold-chrome-gpu-diffserver:8000",
diff_server_http: "gold-chrome-gpu-diffserver:8001",
flaky_trace_threshold: 10000, // no trace is flaky
frontend: {
baseRepoURL: "<inherited from git_repo_url>",
- crsTemplate: "<inherited from crs_url_template>",
defaultCorpus: "chrome-gpu",
title: "Chrome GPU Gold",
},
- gerrit_url: "https://chromium-review.googlesource.com",
lit_html_path: "/usr/local/share/skiacorrectness",
num_commits: 512,
port: ":8000",
diff --git a/golden/k8s-instances/chrome-gpu/chrome-gpu.json5 b/golden/k8s-instances/chrome-gpu/chrome-gpu.json5
index 376a97a..112c112 100644
--- a/golden/k8s-instances/chrome-gpu/chrome-gpu.json5
+++ b/golden/k8s-instances/chrome-gpu/chrome-gpu.json5
@@ -1,11 +1,18 @@
{
bt_instance: "production",
bt_project_id: "skia-public",
+ code_review_systems: [
+ {
+ id: "gerrit",
+ flavor: "gerrit",
+ gerrit_url: "https://chromium-review.googlesource.com",
+ url_template: "https://chromium-review.googlesource.com/%s"
+ },
+ ],
gcs_bucket: "skia-gold-chrome-gpu",
git_bt_table: "git-repos2",
git_repo_url: "https://chromium.googlesource.com/chromium/src",
fs_namespace: "chrome-gpu",
fs_project_id: "skia-firestore",
known_hashes_gcs_path: "skia-gold-chrome-gpu/hash_files/gold-chrome-gpu-hashes.txt",
- primary_crs: "gerrit",
}
diff --git a/golden/k8s-instances/chrome-public/chrome-public-skiacorrectness.json5 b/golden/k8s-instances/chrome-public/chrome-public-skiacorrectness.json5
index ca9e3c4..8a4ac80 100644
--- a/golden/k8s-instances/chrome-public/chrome-public-skiacorrectness.json5
+++ b/golden/k8s-instances/chrome-public/chrome-public-skiacorrectness.json5
@@ -6,17 +6,14 @@
],
cl_comment_template: "<ignored>",
client_secret_file: "/etc/skia.org/login.json",
- crs_url_template: "https://chromium-review.googlesource.com/%s",
diff_server_grpc: "gold-chrome-diffserver:8000",
diff_server_http: "gold-chrome-diffserver:8001",
flaky_trace_threshold: 10000, // no trace is flaky
frontend: {
baseRepoURL: "<inherited from git_repo_url>",
- crsTemplate: "<inherited from crs_url_template>",
defaultCorpus: "android-render-tests",
title: "Chrome Public Gold",
},
- gerrit_url: "https://chromium-review.googlesource.com",
lit_html_path: "/usr/local/share/skiacorrectness",
num_commits: 512,
port: ":8000",
diff --git a/golden/k8s-instances/chrome-public/chrome-public.json5 b/golden/k8s-instances/chrome-public/chrome-public.json5
index 77fa644..b1de6b6 100644
--- a/golden/k8s-instances/chrome-public/chrome-public.json5
+++ b/golden/k8s-instances/chrome-public/chrome-public.json5
@@ -1,11 +1,18 @@
{
bt_instance: "production",
bt_project_id: "skia-public",
+ code_review_systems: [
+ {
+ id: "gerrit",
+ flavor: "gerrit",
+ gerrit_url: "https://chromium-review.googlesource.com",
+ url_template: "https://chromium-review.googlesource.com/%s"
+ },
+ ],
gcs_bucket: "skia-gold-chrome",
git_bt_table: "git-repos2",
git_repo_url: "https://chromium.googlesource.com/chromium/src",
fs_namespace: "chrome",
fs_project_id: "skia-firestore",
known_hashes_gcs_path: "skia-gold-chrome/hash_files/gold-chrome-hashes.txt",
- primary_crs: "gerrit",
}
diff --git a/golden/k8s-instances/chrome/chrome-ingestion-bt.json5 b/golden/k8s-instances/chrome/chrome-ingestion-bt.json5
index 5f254ea..71e2737 100644
--- a/golden/k8s-instances/chrome/chrome-ingestion-bt.json5
+++ b/golden/k8s-instances/chrome/chrome-ingestion-bt.json5
@@ -36,8 +36,10 @@
FirestoreProjectID: "skia-firestore",
FirestoreNamespace: "chrome",
- CodeReviewSystem: "gerrit",
- GerritURL: "https://chromium-review.googlesource.com",
+ // TODO(kjlubick) this is clunky; it would be preferable to use CodeReviewSystems
+ CodeReviewSystems: "gerrit,gerrit-internal",
+ GerritURL: "https://chromium-review.googlesource.com",
+ GerritInternalURL: "https://chrome-internal-review.googlesource.com",
ContinuousIntegrationSystems: "buildbucket",
}
diff --git a/golden/k8s-instances/chrome/chrome-skiacorrectness.json5 b/golden/k8s-instances/chrome/chrome-skiacorrectness.json5
index 26aba21..0e768c3 100644
--- a/golden/k8s-instances/chrome/chrome-skiacorrectness.json5
+++ b/golden/k8s-instances/chrome/chrome-skiacorrectness.json5
@@ -13,18 +13,15 @@
If all the trybots passed and you don't expect your CL to have any effect on browser UI, you can likely ignore this message.\n\
See the FAQ for more information: https://docs.google.com/document/d/1BnwcxzhT8FFvY3YF-6BT4Mqgrb9U40t0HMfEVSSEpNs/edit?usp=sharing\n",
client_secret_file: "/etc/skia.org/login.json",
- crs_url_template: "https://chromium-review.googlesource.com/%s",
diff_server_grpc: "gold-chrome-diffserver:8000",
diff_server_http: "gold-chrome-diffserver:8001",
flaky_trace_threshold: 10000, // no trace is flaky
force_login: true, // This instance requires authentication. It has a public view (chrome-public)
frontend: {
baseRepoURL: "<inherited from git_repo_url>",
- crsTemplate: "<inherited from crs_url_template>",
defaultCorpus: "gtest-pixeltests",
title: "Chrome Gold",
},
- gerrit_url: "https://chromium-review.googlesource.com",
lit_html_path: "/usr/local/share/skiacorrectness",
num_commits: 512,
port: ":8000",
diff --git a/golden/k8s-instances/chrome/chrome.json5 b/golden/k8s-instances/chrome/chrome.json5
index 77fa644..8d409d8 100644
--- a/golden/k8s-instances/chrome/chrome.json5
+++ b/golden/k8s-instances/chrome/chrome.json5
@@ -1,11 +1,23 @@
{
bt_instance: "production",
bt_project_id: "skia-public",
+ code_review_systems: [
+ {
+ id: "gerrit", // public reviews
+ flavor: "gerrit",
+ gerrit_url: "https://chromium-review.googlesource.com",
+ url_template: "https://chromium-review.googlesource.com/%s"
+ }, {
+ id: "gerrit-internal", // internal reviews
+ flavor: "gerrit",
+ gerrit_url: "https://chrome-internal-review.googlesource.com",
+ url_template: "https://chrome-internal-review.googlesource.com/%s"
+ }
+ ],
gcs_bucket: "skia-gold-chrome",
git_bt_table: "git-repos2",
git_repo_url: "https://chromium.googlesource.com/chromium/src",
fs_namespace: "chrome",
fs_project_id: "skia-firestore",
known_hashes_gcs_path: "skia-gold-chrome/hash_files/gold-chrome-hashes.txt",
- primary_crs: "gerrit",
}
diff --git a/golden/k8s-instances/flutter-engine/flutter-engine-ingestion-bt.json5 b/golden/k8s-instances/flutter-engine/flutter-engine-ingestion-bt.json5
index 513cd11..cd277ac 100644
--- a/golden/k8s-instances/flutter-engine/flutter-engine-ingestion-bt.json5
+++ b/golden/k8s-instances/flutter-engine/flutter-engine-ingestion-bt.json5
@@ -36,7 +36,7 @@
FirestoreProjectID: "skia-firestore",
FirestoreNamespace: "flutter-engine",
- CodeReviewSystem: "github",
+ CodeReviewSystems: "github",
GitHubCredentialsPath: "/var/secrets/github/github_token",
GitHubRepo: "flutter/engine",
diff --git a/golden/k8s-instances/flutter-engine/flutter-engine-skiacorrectness.json5 b/golden/k8s-instances/flutter-engine/flutter-engine-skiacorrectness.json5
index 7470bba..42dbf31 100644
--- a/golden/k8s-instances/flutter-engine/flutter-engine-skiacorrectness.json5
+++ b/golden/k8s-instances/flutter-engine/flutter-engine-skiacorrectness.json5
@@ -5,18 +5,14 @@
cl_comment_template: "Gold has detected about {{.NumUntriaged}} untriaged digest(s) on patchset {{.PatchSetOrder}}.\n\
View them at {{.InstanceURL}}/cl/{{.CRS}}/{{.ChangeListID}}",
client_secret_file: "/etc/skia.org/login.json",
- crs_url_template: "https://github.com/flutter/engine/pull/%s",
diff_server_grpc: "gold-flutter-engine-diffserver:8000",
diff_server_http: "gold-flutter-engine-diffserver:8001",
flaky_trace_threshold: 10000, // no trace is flaky
frontend: {
baseRepoURL: "<inherited from git_repo_url>",
- crsTemplate: "<inherited from crs_url_template>",
defaultCorpus: "flutter-engine",
title: "Flutter Engine Gold",
},
- github_cred_path: "/var/secrets/github/github_token",
- github_repo: "flutter/engine",
lit_html_path: "/usr/local/share/skiacorrectness",
num_commits: 200,
port: ":8000",
diff --git a/golden/k8s-instances/flutter-engine/flutter-engine.json5 b/golden/k8s-instances/flutter-engine/flutter-engine.json5
index 00f34c8..2a2f53b 100644
--- a/golden/k8s-instances/flutter-engine/flutter-engine.json5
+++ b/golden/k8s-instances/flutter-engine/flutter-engine.json5
@@ -1,11 +1,19 @@
{
bt_instance: "production",
bt_project_id: "skia-public",
+ code_review_systems: [
+ {
+ id: "github",
+ flavor: "github",
+ github_cred_path: "/var/secrets/github/github_token",
+ github_repo: "flutter/engine",
+ url_template: "https://github.com/flutter/engine/pull/%s"
+ },
+ ],
gcs_bucket: "skia-gold-flutter-engine",
git_bt_table: "git-repos2",
git_repo_url: "https://github.com/flutter/engine",
fs_namespace: "flutter-engine",
fs_project_id: "skia-firestore",
known_hashes_gcs_path: "skia-gold-flutter-engine/hash_files/gold-flutter-engine-hashes.txt",
- primary_crs: "github",
}
diff --git a/golden/k8s-instances/flutter/flutter-ingestion-bt.json5 b/golden/k8s-instances/flutter/flutter-ingestion-bt.json5
index e11b8ca..f0bcca5 100644
--- a/golden/k8s-instances/flutter/flutter-ingestion-bt.json5
+++ b/golden/k8s-instances/flutter/flutter-ingestion-bt.json5
@@ -36,7 +36,7 @@
FirestoreProjectID: "skia-firestore",
FirestoreNamespace: "flutter",
- CodeReviewSystem: "github",
+ CodeReviewSystems: "github",
GitHubCredentialsPath: "/var/secrets/github/github_token",
GitHubRepo: "flutter/flutter",
diff --git a/golden/k8s-instances/flutter/flutter-skiacorrectness.json5 b/golden/k8s-instances/flutter/flutter-skiacorrectness.json5
index 86b44d3..a7ab27f 100644
--- a/golden/k8s-instances/flutter/flutter-skiacorrectness.json5
+++ b/golden/k8s-instances/flutter/flutter-skiacorrectness.json5
@@ -5,18 +5,14 @@
cl_comment_template: "Gold has detected about {{.NumUntriaged}} untriaged digest(s) on patchset {{.PatchSetOrder}}.\n\
View them at {{.InstanceURL}}/cl/{{.CRS}}/{{.ChangeListID}}",
client_secret_file: "/etc/skia.org/login.json",
- crs_url_template: "https://github.com/flutter/flutter/pull/%s",
diff_server_grpc: "gold-flutter-diffserver:8000",
diff_server_http: "gold-flutter-diffserver:8001",
flaky_trace_threshold: 10000, // no trace is flaky
frontend: {
baseRepoURL: "<inherited from git_repo_url>",
- crsTemplate: "<inherited from crs_url_template>",
defaultCorpus: "flutter",
title: "Flutter Gold",
},
- github_cred_path: "/var/secrets/github/github_token",
- github_repo: "flutter/flutter",
lit_html_path: "/usr/local/share/skiacorrectness",
num_commits: 200,
port: ":8000",
diff --git a/golden/k8s-instances/flutter/flutter.json5 b/golden/k8s-instances/flutter/flutter.json5
index 827c540..10adfe6 100644
--- a/golden/k8s-instances/flutter/flutter.json5
+++ b/golden/k8s-instances/flutter/flutter.json5
@@ -1,11 +1,19 @@
{
bt_instance: "production",
bt_project_id: "skia-public",
+ code_review_systems: [
+ {
+ id: "github",
+ flavor: "github",
+ github_cred_path: "/var/secrets/github/github_token",
+ github_repo: "flutter/flutter",
+ url_template: "https://github.com/flutter/flutter/pull/%s"
+ },
+ ],
gcs_bucket: "skia-gold-flutter",
git_bt_table: "git-repos2",
git_repo_url: "https://github.com/flutter/flutter",
fs_namespace: "flutter",
fs_project_id: "skia-firestore",
known_hashes_gcs_path: "skia-gold-flutter/hash_files/gold-flutter-hashes.txt",
- primary_crs: "github",
}
diff --git a/golden/k8s-instances/fuchsia-public/fuchsia-public-ingestion-bt.json5 b/golden/k8s-instances/fuchsia-public/fuchsia-public-ingestion-bt.json5
index 1836c96..adc4366 100644
--- a/golden/k8s-instances/fuchsia-public/fuchsia-public-ingestion-bt.json5
+++ b/golden/k8s-instances/fuchsia-public/fuchsia-public-ingestion-bt.json5
@@ -36,7 +36,7 @@
FirestoreProjectID: "skia-firestore",
FirestoreNamespace: "fuchsia-public",
- CodeReviewSystem: "gerrit",
+ CodeReviewSystems: "gerrit",
GerritURL: "https://fuchsia-review.googlesource.com",
ContinuousIntegrationSystems: "buildbucket",
diff --git a/golden/k8s-instances/fuchsia-public/fuchsia-public-skiacorrectness.json5 b/golden/k8s-instances/fuchsia-public/fuchsia-public-skiacorrectness.json5
index 4d15073..0c0ba17 100644
--- a/golden/k8s-instances/fuchsia-public/fuchsia-public-skiacorrectness.json5
+++ b/golden/k8s-instances/fuchsia-public/fuchsia-public-skiacorrectness.json5
@@ -5,18 +5,15 @@
cl_comment_template: "Gold has detected about {{.NumUntriaged}} untriaged digest(s) on patchset {{.PatchSetOrder}}.\n\
View them at {{.InstanceURL}}/cl/{{.CRS}}/{{.ChangeListID}}",
client_secret_file: "/etc/skia.org/login.json",
- crs_url_template: "https://fuchsia-review.googlesource.com/%s",
diff_server_grpc: "gold-fuchsia-public-diffserver:8000",
diff_server_http: "gold-fuchsia-public-diffserver:8001",
flaky_trace_threshold: 10000, // no trace is flaky
force_login: true, // This instance requires authentication.
frontend: {
baseRepoURL: "<inherited from git_repo_url>",
- crsTemplate: "<inherited from crs_url_template>",
defaultCorpus: "test",
title: "Fuchsia Gold",
},
- gerrit_url: "https://fuchsia-review.googlesource.com/",
lit_html_path: "/usr/local/share/skiacorrectness",
num_commits: 200,
port: ":8000",
diff --git a/golden/k8s-instances/fuchsia-public/fuchsia-public.json5 b/golden/k8s-instances/fuchsia-public/fuchsia-public.json5
index 4658611..799e53e 100644
--- a/golden/k8s-instances/fuchsia-public/fuchsia-public.json5
+++ b/golden/k8s-instances/fuchsia-public/fuchsia-public.json5
@@ -1,11 +1,18 @@
{
bt_instance: "production",
bt_project_id: "skia-public",
+ code_review_systems: [
+ {
+ id: "gerrit",
+ flavor: "gerrit",
+ gerrit_url: "https://fuchsia-review.googlesource.com/",
+ url_template: "https://fuchsia-review.googlesource.com/%s"
+ },
+ ],
gcs_bucket: "skia-gold-fuchsia-public",
git_bt_table: "git-repos2",
git_repo_url: "https://fuchsia.googlesource.com/fuchsia",
fs_namespace: "fuchsia-public",
fs_project_id: "skia-firestore",
known_hashes_gcs_path: "skia-gold-fuchsia-public/hash_files/gold-fuchsia-public-hashes.txt",
- primary_crs: "gerrit",
}
diff --git a/golden/k8s-instances/lottie/lottie-skiacorrectness.json5 b/golden/k8s-instances/lottie/lottie-skiacorrectness.json5
index 9db377e..44adf0a 100644
--- a/golden/k8s-instances/lottie/lottie-skiacorrectness.json5
+++ b/golden/k8s-instances/lottie/lottie-skiacorrectness.json5
@@ -5,17 +5,14 @@
cl_comment_template: "Gold has detected about {{.NumUntriaged}} untriaged digest(s) on patchset {{.PatchSetOrder}}.\n\
View them at {{.InstanceURL}}/cl/{{.CRS}}/{{.ChangeListID}}",
client_secret_file: "/etc/skia.org/login.json",
- crs_url_template: "https://skia-review.googlesource.com/%s",
diff_server_grpc: "gold-lottie-diffserver:8000",
diff_server_http: "gold-lottie-diffserver:8001",
flaky_trace_threshold: 10000, // no flaky traces
frontend: {
baseRepoURL: "<inherited from git_repo_url>",
- crsTemplate: "<inherited from crs_url_template>",
defaultCorpus: "lottie",
title: "Lottie Gold",
},
- gerrit_url: "https://skia-review.googlesource.com",
lit_html_path: "/usr/local/share/skiacorrectness",
num_commits: 500,
port: ":8000",
diff --git a/golden/k8s-instances/lottie/lottie.json5 b/golden/k8s-instances/lottie/lottie.json5
index 6d1f360..4f3c77b 100644
--- a/golden/k8s-instances/lottie/lottie.json5
+++ b/golden/k8s-instances/lottie/lottie.json5
@@ -1,11 +1,18 @@
{
bt_instance: "production",
bt_project_id: "skia-public",
+ code_review_systems: [
+ {
+ id: "gerrit",
+ flavor: "gerrit",
+ gerrit_url: "https://skia-review.googlesource.com",
+ url_template: "https://skia-review.googlesource.com/%s"
+ },
+ ],
gcs_bucket: "skia-gold-lottie",
git_bt_table: "git-repos2",
git_repo_url: "https://skia.googlesource.com/lottie-ci",
fs_namespace: "lottie",
fs_project_id: "skia-firestore",
known_hashes_gcs_path: "skia-gold-lottie/hash_files/gold-lottie-hashes.txt",
- primary_crs: "gerrit",
}
diff --git a/golden/k8s-instances/pdfium/pdfium-ingestion-bt.json5 b/golden/k8s-instances/pdfium/pdfium-ingestion-bt.json5
index 13cce85..42c899e 100644
--- a/golden/k8s-instances/pdfium/pdfium-ingestion-bt.json5
+++ b/golden/k8s-instances/pdfium/pdfium-ingestion-bt.json5
@@ -36,7 +36,7 @@
FirestoreProjectID: "skia-firestore",
FirestoreNamespace: "pdfium",
- CodeReviewSystem: "gerrit",
+ CodeReviewSystems: "gerrit",
GerritURL: "https://pdfium-review.googlesource.com",
ContinuousIntegrationSystems: "buildbucket",
diff --git a/golden/k8s-instances/pdfium/pdfium-skiacorrectness.json5 b/golden/k8s-instances/pdfium/pdfium-skiacorrectness.json5
index cfa6374..318ce90 100644
--- a/golden/k8s-instances/pdfium/pdfium-skiacorrectness.json5
+++ b/golden/k8s-instances/pdfium/pdfium-skiacorrectness.json5
@@ -5,17 +5,14 @@
cl_comment_template: "Gold has detected about {{.NumUntriaged}} untriaged digest(s) on patchset {{.PatchSetOrder}}.\n\
View them at {{.InstanceURL}}/cl/{{.CRS}}/{{.ChangeListID}}",
client_secret_file: "/etc/skia.org/login.json",
- crs_url_template: "https://pdfium-review.googlesource.com/%s",
diff_server_grpc: "gold-pdfium-diffserver:8000",
diff_server_http: "gold-pdfium-diffserver:8001",
flaky_trace_threshold: 10000, // no flaky traces
frontend: {
baseRepoURL: "<inherited from git_repo_url>",
- crsTemplate: "<inherited from crs_url_template>",
defaultCorpus: "corpus",
title: "Pdfium Gold",
},
- gerrit_url: "https://pdfium-review.googlesource.com",
lit_html_path: "/usr/local/share/skiacorrectness",
num_commits: 500,
port: ":8000",
diff --git a/golden/k8s-instances/pdfium/pdfium.json5 b/golden/k8s-instances/pdfium/pdfium.json5
index bcfd95d..1d19136 100644
--- a/golden/k8s-instances/pdfium/pdfium.json5
+++ b/golden/k8s-instances/pdfium/pdfium.json5
@@ -1,11 +1,18 @@
{
bt_instance: "production",
bt_project_id: "skia-public",
+ code_review_systems: [
+ {
+ id: "gerrit",
+ flavor: "gerrit",
+ gerrit_url: "https://pdfium-review.googlesource.com",
+ url_template: "https://pdfium-review.googlesource.com/%s"
+ },
+ ],
gcs_bucket: "skia-pdfium-gm", // Legacy bucket name
git_bt_table: "git-repos2",
git_repo_url: "https://pdfium.googlesource.com/pdfium",
fs_namespace: "pdfium",
fs_project_id: "skia-firestore",
known_hashes_gcs_path: "skia-gold-pdfium/hash_files/gold-pdfium-hashes.txt",
- primary_crs: "gerrit",
}
diff --git a/golden/k8s-instances/skia-infra/skia-infra-ingestion-bt.json5 b/golden/k8s-instances/skia-infra/skia-infra-ingestion-bt.json5
index 237feb7..001f516 100644
--- a/golden/k8s-instances/skia-infra/skia-infra-ingestion-bt.json5
+++ b/golden/k8s-instances/skia-infra/skia-infra-ingestion-bt.json5
@@ -36,7 +36,7 @@
FirestoreProjectID: "skia-firestore",
FirestoreNamespace: "skia-infra",
- CodeReviewSystem: "gerrit",
+ CodeReviewSystems: "gerrit",
GerritURL: "https://skia-review.googlesource.com",
ContinuousIntegrationSystems: "buildbucket",
diff --git a/golden/k8s-instances/skia-infra/skia-infra-skiacorrectness.json5 b/golden/k8s-instances/skia-infra/skia-infra-skiacorrectness.json5
index 09d1175..976e20d 100644
--- a/golden/k8s-instances/skia-infra/skia-infra-skiacorrectness.json5
+++ b/golden/k8s-instances/skia-infra/skia-infra-skiacorrectness.json5
@@ -5,17 +5,14 @@
cl_comment_template: "Gold has detected about {{.NumUntriaged}} untriaged digest(s) on patchset {{.PatchSetOrder}}.\n\
View them at {{.InstanceURL}}/cl/{{.CRS}}/{{.ChangeListID}}",
client_secret_file: "/etc/skia.org/login.json",
- crs_url_template: "https://skia-review.googlesource.com/%s",
diff_server_grpc: "gold-skia-infra-diffserver:8000",
diff_server_http: "gold-skia-infra-diffserver:8001",
flaky_trace_threshold: 10,
frontend: {
baseRepoURL: "<inherited from git_repo_url>",
- crsTemplate: "<inherited from crs_url_template>",
defaultCorpus: "infra",
title: "Skia Infra Gold",
},
- gerrit_url: "https://skia-review.googlesource.com",
lit_html_path: "/usr/local/share/skiacorrectness",
negatives_max_age: "4320h", // 180 days
num_commits: 200,
diff --git a/golden/k8s-instances/skia-infra/skia-infra.json5 b/golden/k8s-instances/skia-infra/skia-infra.json5
index c5f283d..3135196 100644
--- a/golden/k8s-instances/skia-infra/skia-infra.json5
+++ b/golden/k8s-instances/skia-infra/skia-infra.json5
@@ -1,11 +1,18 @@
{
bt_instance: "production",
bt_project_id: "skia-public",
+ code_review_systems: [
+ {
+ id: "gerrit",
+ flavor: "gerrit",
+ gerrit_url: "https://skia-review.googlesource.com",
+ url_template: "https://skia-review.googlesource.com/%s"
+ },
+ ],
gcs_bucket: "skia-gold-skia-infra",
git_bt_table: "git-repos2",
git_repo_url: "https://skia.googlesource.com/buildbot.git",
fs_namespace: "skia-infra",
fs_project_id: "skia-firestore",
known_hashes_gcs_path: "skia-gold-skia-infra/hash_files/gold-skia-infra-hashes.txt",
- primary_crs: "gerrit",
}
diff --git a/golden/k8s-instances/skia-public/skia-public-skiacorrectness.json5 b/golden/k8s-instances/skia-public/skia-public-skiacorrectness.json5
index 2200fb9..7cf05a3 100644
--- a/golden/k8s-instances/skia-public/skia-public-skiacorrectness.json5
+++ b/golden/k8s-instances/skia-public/skia-public-skiacorrectness.json5
@@ -7,17 +7,14 @@
],
cl_comment_template: "<ignored>",
client_secret_file: "/etc/skia.org/login.json",
- crs_url_template: "https://skia-review.googlesource.com/%s",
diff_server_grpc: "gold-skia-diffserver:8000",
diff_server_http: "gold-skia-diffserver:8001",
flaky_trace_threshold: 10,
frontend: {
baseRepoURL: "<inherited from git_repo_url>",
- crsTemplate: "<inherited from crs_url_template>",
defaultCorpus: "gm",
title: "Skia Public Gold",
},
- gerrit_url: "https://skia-review.googlesource.com",
lit_html_path: "/usr/local/share/skiacorrectness",
num_commits: 256,
port: ":8000",
diff --git a/golden/k8s-instances/skia-public/skia-public.json5 b/golden/k8s-instances/skia-public/skia-public.json5
index e0dac98..5ad917e 100644
--- a/golden/k8s-instances/skia-public/skia-public.json5
+++ b/golden/k8s-instances/skia-public/skia-public.json5
@@ -1,11 +1,18 @@
{
bt_instance: "production",
bt_project_id: "skia-public",
+ code_review_systems: [
+ {
+ id: "gerrit",
+ flavor: "gerrit",
+ gerrit_url: "https://skia-review.googlesource.com",
+ url_template: "https://skia-review.googlesource.com/%s"
+ },
+ ],
gcs_bucket: "skia-infra-gm",
git_bt_table: "git-repos2",
git_repo_url: "https://skia.googlesource.com/skia.git",
fs_namespace: "skia",
fs_project_id: "skia-firestore",
known_hashes_gcs_path: "skia-infra-gm/hash_files/gold-prod-hashes.txt",
- primary_crs: "gerrit",
}
diff --git a/golden/k8s-instances/skia/skia-ingestion-bt.json5 b/golden/k8s-instances/skia/skia-ingestion-bt.json5
index 028dcd5..d45de1b 100644
--- a/golden/k8s-instances/skia/skia-ingestion-bt.json5
+++ b/golden/k8s-instances/skia/skia-ingestion-bt.json5
@@ -36,7 +36,7 @@
FirestoreProjectID: "skia-firestore",
FirestoreNamespace: "skia",
- CodeReviewSystem: "gerrit",
+ CodeReviewSystems: "gerrit",
GerritURL: "https://skia-review.googlesource.com",
ContinuousIntegrationSystems: "buildbucket",
diff --git a/golden/k8s-instances/skia/skia-skiacorrectness.json5 b/golden/k8s-instances/skia/skia-skiacorrectness.json5
index e58be6e..f3505ad 100644
--- a/golden/k8s-instances/skia/skia-skiacorrectness.json5
+++ b/golden/k8s-instances/skia/skia-skiacorrectness.json5
@@ -5,18 +5,15 @@
cl_comment_template: "Gold has detected about {{.NumUntriaged}} untriaged digest(s) on patchset {{.PatchSetOrder}}.\n\
View them at {{.InstanceURL}}/cl/{{.CRS}}/{{.ChangeListID}}",
client_secret_file: "/etc/skia.org/login.json",
- crs_url_template: "https://skia-review.googlesource.com/%s",
diff_server_grpc: "gold-skia-diffserver:8000",
diff_server_http: "gold-skia-diffserver:8001",
flaky_trace_threshold: 10,
force_login: true, // This instance requires authentication. It has a public view (skia-public)
frontend: {
baseRepoURL: "<inherited from git_repo_url>",
- crsTemplate: "<inherited from crs_url_template>",
defaultCorpus: "gm",
title: "Skia Gold",
},
- gerrit_url: "https://skia-review.googlesource.com",
lit_html_path: "/usr/local/share/skiacorrectness",
negatives_max_age: "4320h", // 180 days
num_commits: 256,
diff --git a/golden/k8s-instances/skia/skia.json5 b/golden/k8s-instances/skia/skia.json5
index 41c322d..8f306fb 100644
--- a/golden/k8s-instances/skia/skia.json5
+++ b/golden/k8s-instances/skia/skia.json5
@@ -1,11 +1,18 @@
{
bt_instance: "production",
bt_project_id: "skia-public",
+ code_review_systems: [
+ {
+ id: "gerrit",
+ flavor: "gerrit",
+ gerrit_url: "https://skia-review.googlesource.com",
+ url_template: "https://skia-review.googlesource.com/%s"
+ },
+ ],
gcs_bucket: "skia-infra-gm", // Legacy bucket name
git_bt_table: "git-repos2",
git_repo_url: "https://skia.googlesource.com/skia.git",
fs_namespace: "skia",
fs_project_id: "skia-firestore",
known_hashes_gcs_path: "skia-infra-gm/hash_files/gold-prod-hashes.txt",
- primary_crs: "gerrit",
}
diff --git a/golden/modules/blamelist-panel-sk/blamelist-panel-sk-demo.ts b/golden/modules/blamelist-panel-sk/blamelist-panel-sk-demo.ts
index e15c6ba..ac99a79 100644
--- a/golden/modules/blamelist-panel-sk/blamelist-panel-sk-demo.ts
+++ b/golden/modules/blamelist-panel-sk/blamelist-panel-sk-demo.ts
@@ -8,7 +8,6 @@
testOnlySetSettings({
baseRepoURL: 'https://github.com/example/example',
- crsTemplate: 'https://skia-review.googlesource.com/%s',
});
let ele = new BlamelistPanelSk();
diff --git a/golden/modules/blamelist-panel-sk/blamelist-panel-sk.ts b/golden/modules/blamelist-panel-sk/blamelist-panel-sk.ts
index b313c70..865a68b 100644
--- a/golden/modules/blamelist-panel-sk/blamelist-panel-sk.ts
+++ b/golden/modules/blamelist-panel-sk/blamelist-panel-sk.ts
@@ -13,7 +13,7 @@
import { diffDate } from 'common-sk/modules/human';
import { ElementSk } from '../../../infra-sk/modules/ElementSk';
import { truncateWithEllipses } from '../common';
-import { baseRepoURL, codeReviewURLTemplate } from '../settings';
+import { baseRepoURL } from '../settings';
const maxCommitsToDisplay = 15;
@@ -45,7 +45,7 @@
// in quotes because we re-use the Commit structure to pass in information about a CL for the
// purpose of drawing a trace.
let newestCommit = commits[0];
- if (newestCommit.is_cl) {
+ if (newestCommit.cl_url) {
newestCommit = commits[1];
}
@@ -65,9 +65,8 @@
};
const commitHref = (commit: Commit) => {
- if (commit.is_cl) {
- const crsTemplate = codeReviewURLTemplate();
- return crsTemplate.replace('%s', commit.hash);
+ if (commit.cl_url) {
+ return commit.cl_url;
}
// TODO(kjlubick): Deduplicate with by-blame-sk.
const repo = baseRepoURL();
@@ -90,7 +89,7 @@
readonly author: string;
readonly message: string;
readonly commit_time: number;
- readonly is_cl: boolean;
+ readonly cl_url: string;
}
export class BlamelistPanelSk extends ElementSk {
@@ -112,7 +111,7 @@
private _commits: Commit[] = [];
private _lastGoodCommit: Commit = {
- hash: '', author: '', message: '', commit_time: 0, is_cl: false
+ hash: '', author: '', message: '', commit_time: 0, cl_url: ''
};
constructor() {
diff --git a/golden/modules/blamelist-panel-sk/demo_data.ts b/golden/modules/blamelist-panel-sk/demo_data.ts
index f24bde5..bd8b2f5 100644
--- a/golden/modules/blamelist-panel-sk/demo_data.ts
+++ b/golden/modules/blamelist-panel-sk/demo_data.ts
@@ -8,133 +8,133 @@
hash: 'dded3c7506efc5635e60ffb7a908cbe8f1f028f1',
author: 'Alfa (alfa@example.com)',
message: 'Update provisioning_profile to unbreak iOS since cert refresh',
- is_cl: false,
+ cl_url: '',
},
{
commit_time: 1584834200,
hash: '44fc53b7f5d8390d7ae228d5fa8bac0b225ed2b9',
author: 'Bravo (bravo@example.com)',
message: 'Add BGR_10A2 support to Ganesh',
- is_cl: false,
+ cl_url: '',
},
{
commit_time: 1584834100,
hash: 'c50f4e9399cc7ae72b05eb641b50a7cd06d5af82',
author: 'Charlie (charlie@example.com)',
message: 'Simplify GrTextBlob::flush',
- is_cl: false,
+ cl_url: '',
},
{
commit_time: 1584834000,
hash: '9145f784f3261f227846e5b08dc2691a888b113c',
author: 'Delta (deltadelta@example.com)',
message: 'Implement D3D copySurface.',
- is_cl: false,
+ cl_url: '',
},
{
commit_time: 1584824000,
hash: '02b91385f58b84dc14cb6f81478b36d7fcb0d24b',
author: 'Echo Foxtrot (ef@example.com)',
message: 'Rename flush -> addOp',
- is_cl: false,
+ cl_url: '',
},
{
commit_time: 1584814000,
hash: 'a1e90aba441b0e68a07e3ab6660cd673335a76f5',
author: 'Golf Golf Golf Golf Golf Golf Golf(golf@example.com)',
message: 'Turn on Vulkan bots for Galaxy S20.',
- is_cl: false,
+ cl_url: '',
},
{
commit_time: 1584804000,
hash: 'b2b9fbc5675c46cb3af30ccd9b6e20bc57ec04c8',
author: 'Hotel India (hotel.ind@example.com)',
message: 'Compare all fields in SkSL::Layout::operator==',
- is_cl: false,
+ cl_url: '',
},
{
commit_time: 1584800000,
hash: 'f1792cde0b2b7b612cac6aa5b9e7a8ce16b9b2cb',
author: 'Hotel India (hotel.ind@example.com)',
message: 'Use constant swizzle syntax in GrDrawVerticesOp',
- is_cl: false,
+ cl_url: '',
},
{
commit_time: 1584780000,
hash: 'fc75d5c52da617b06c7566c9e389587edde3f284',
author: 'Juliett Kilo (julia.kilo@example.com)',
message: '[skottie] Brightness and Contrast effect',
- is_cl: false,
+ cl_url: '',
},
{
commit_time: 1584760000,
hash: 'e15088801e9722d9a320463554a3a21839213b48',
author: 'Mike Lima (lima@example.com)',
message: 'detect failed matrix update in SkDraw::drawAtlas()',
- is_cl: false,
+ cl_url: '',
},
{
commit_time: 1584760000,
hash: '548de7451e752ce0d6fd842c1bb4f04af4c0afdc',
author: 'Hotel India (hotel.ind@example.com)',
message: 'Change Marker IDs to be strings',
- is_cl: false,
+ cl_url: '',
},
{
commit_time: 1584740000,
hash: '7676f4ecf3ec2f544701465faca1d33af1489822',
author: 'Zulu (zulu@example.com)',
message: 'Remove SkFrontBufferedStream',
- is_cl: false,
+ cl_url: '',
},
{
commit_time: 1584720000,
hash: '4baa7326ccfbfaf66e25d91628378536d4999999',
author: 'Papa November (pnov@example.com)',
message: '[infra] Add POC task driver',
- is_cl: false,
+ cl_url: '',
},
{
commit_time: 1584700000,
hash: 'f5132a05c893a86f8bf26bcf3253985d9973fea2',
author: 'Quebec (quebec@example.com)',
message: "Reland \"Optimize GrTessellatePathOp's code to emit inner tri",
- is_cl: false,
+ cl_url: '',
},
{
commit_time: 1584600005,
hash: '3d599d16ecd8009dfb185522e87c5c1cf8a47057',
author: 'skia-recreate-skps (skia-recreate-skps@example.com)',
message: 'Update Go Deps',
- is_cl: false,
+ cl_url: '',
},
{
commit_time: 1584600005,
hash: '7a0517f8bdc0d168565a2944f0c14db06084384d',
author: 'skia-autoroll (skia-autoroll@example.com)',
message: 'Roll third_party/externals/angle2 3cb9c4bee9b3..4395170e6091',
- is_cl: false,
+ cl_url: '',
},
{
commit_time: 1584600003,
hash: 'd18100afb62401b49e0b3a5c87ed8fa006e8d3a6',
author: 'skia-autoroll (skia-autoroll@example.com)',
message: 'Roll ../src 9781ff27c9e9..78824aa9d99f (468 commits)',
- is_cl: false,
+ cl_url: '',
},
{
commit_time: 1584600002,
hash: '2ac6fa0ae796b7f050bd8e8ad60890427a4ec15e',
author: 'skia-autoroll (skia-autoroll@example.com)',
message: 'Roll third_party/externals/swiftshader 60aa34a990fa..2717702',
- is_cl: false,
+ cl_url: '',
},
{
commit_time: 1584600001,
hash: '667edf14ad72966ec36aa6cd705b98cb7d7eee28',
author: 'skia-autoroll (skia-autoroll@example.com)',
message: 'Roll third_party/externals/dawn 00b90ea83262..88f2ec853f80 (',
- is_cl: false,
+ cl_url: '',
},
];
@@ -144,13 +144,13 @@
hash: '12345',
author: 'skia-autoroll (skia-autoroll@example.com)',
message: 'Roll third_party/externals/dawn 00b90ea83262..88f2ec853f80 (',
- is_cl: true,
+ cl_url: 'https://skia-review.googlesource.com/12345',
},
{
commit_time: 1584835000,
hash: 'dded3c7506efc5635e60ffb7a908cbe8f1f028f1',
author: 'Alfa (alfa@example.com)',
message: 'Update provisioning_profile to unbreak iOS since cert refresh',
- is_cl: false,
+ cl_url: '',
},
];
diff --git a/golden/modules/rpc_types.ts b/golden/modules/rpc_types.ts
index 94d31e2..7c85b71 100644
--- a/golden/modules/rpc_types.ts
+++ b/golden/modules/rpc_types.ts
@@ -82,7 +82,7 @@
hash: string;
author: string;
message: string;
- is_cl: boolean;
+ cl_url: string;
}
export interface TraceComment {
diff --git a/golden/modules/settings.js b/golden/modules/settings.js
index ec0029e..132be92 100644
--- a/golden/modules/settings.js
+++ b/golden/modules/settings.js
@@ -28,13 +28,6 @@
}
/**
- * @return {string}
- */
-export function codeReviewURLTemplate() {
- return window.GoldSettings && window.GoldSettings.crsTemplate;
-}
-
-/**
* @param newSettings {Object}
*/
export function testOnlySetSettings(newSettings) {
