[goldctl] add --bucket flag to override GCS bucket
This removes the formulaic transforms for skia (which
does not currently use goldctl) and adds tests for
URL overriding and bucket overriding.
Change-Id: I3e56b84daadb63a0d304d673fc620b523bb79bab
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/368237
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
diff --git a/gold-client/cmd/goldctl/cmd_dump_test.go b/gold-client/cmd/goldctl/cmd_dump_test.go
index 738ce58..cb1a695 100644
--- a/gold-client/cmd/goldctl/cmd_dump_test.go
+++ b/gold-client/cmd/goldctl/cmd_dump_test.go
@@ -13,7 +13,7 @@
workDir := t.TempDir()
setupAuthWithGSUtil(t, workDir)
- mh := mockRPCResponses().Positive("pixel-tests", blankDigest).
+ mh := mockRPCResponses("https://my-instance-gold.skia.org").Positive("pixel-tests", blankDigest).
Negative("other-test", blankDigest).
Known("11111111111111111111111111111111").Build()
diff --git a/gold-client/cmd/goldctl/cmd_imgtest.go b/gold-client/cmd/goldctl/cmd_imgtest.go
index 6e033d2..002bb05 100644
--- a/gold-client/cmd/goldctl/cmd_imgtest.go
+++ b/gold-client/cmd/goldctl/cmd_imgtest.go
@@ -19,18 +19,19 @@
// Specifically, it houses the flags.
type imgTest struct {
// Common flags.
+ bucketOverride string
+ changelistID string
codeReviewSystem string
- continuousIntegrationSystem string
commitHash string
+ continuousIntegrationSystem string
corpus string
failureFile string
instanceID string
- changelistID string
- tryJobID string
keysFile string
passFailStep bool
- patchsetOrder int
patchsetID string
+ patchsetOrder int
+ tryJobID string
uploadOnly bool
urlOverride string
workDir string
@@ -118,8 +119,9 @@
imgTestCheckCmd.Flags().StringVar(&env.pngFile, "png-file", "", "Path to the PNG file that contains the test results.")
imgTestCheckCmd.Flags().StringVar(&env.instanceID, "instance", "", "ID of the Gold instance.")
+ imgTestCheckCmd.Flags().StringVar(&env.bucketOverride, "bucket", "", "GCS Bucket to use. If empty the URL will be derived from the value of 'instance'")
imgTestCheckCmd.Flags().StringVar(&env.changelistID, "changelist", "", "If provided, the ChangelistExpectations matching this will apply.")
- imgTestCheckCmd.Flags().StringVar(&env.urlOverride, "url", "", "URL of the Gold instance. Used for testing, if empty the URL will be derived from the value of 'instance'")
+ imgTestCheckCmd.Flags().StringVar(&env.urlOverride, "url", "", "URL of the Gold instance. If empty the URL will be derived from the value of 'instance'")
must(imgTestCheckCmd.MarkFlagRequired(fstrWorkDir))
must(imgTestCheckCmd.MarkFlagRequired("test-name"))
@@ -142,6 +144,7 @@
cmd.Flags().BoolVar(&i.passFailStep, "passfail", false, "Whether the 'add' call returns a pass/fail for each test.")
cmd.Flags().BoolVar(&i.uploadOnly, "upload-only", false, "Skip reading expectations from the server. Incompatible with passfail=true.")
+ cmd.Flags().StringVar(&i.bucketOverride, "bucket", "", "GCS Bucket to write to. If empty the URL will be derived from the value of 'instance'")
cmd.Flags().StringVar(&i.changelistID, "changelist", "", "Changelist ID if this is run as a TryJob.")
cmd.Flags().StringVar(&i.codeReviewSystem, "crs", "", "CodeReviewSystem, if any (e.g. 'gerrit', 'github')")
cmd.Flags().StringVar(&i.commitHash, "commit", "", "Git commit hash")
@@ -152,7 +155,7 @@
cmd.Flags().IntVar(&i.patchsetOrder, "patchset", 0, "Patchset number if this is run as a TryJob.")
cmd.Flags().StringVar(&i.patchsetID, "patchset_id", "", "Patchset id (e.g. githash) if this is run as a TryJob.")
cmd.Flags().StringVar(&i.tryJobID, "jobid", "", "TryJob ID if this is a TryJob run.")
- cmd.Flags().StringVar(&i.urlOverride, "url", "", "URL of the Gold instance. Used for testing, if empty the URL will be derived from the value of 'instance'")
+ cmd.Flags().StringVar(&i.urlOverride, "url", "", "URL of the Gold instance. If empty the URL will be derived from the value of 'instance'")
cmd.Flags().StringVar(&i.changelistID, "issue", "", "[deprecated] Gerrit issue if this is trybot run. ")
must(cmd.MarkFlagRequired(fstrWorkDir))
@@ -192,9 +195,10 @@
if err != nil {
logErrf(ctx, "Could not load existing run, trying to initialize %s\n%s\n", i.workDir, err)
config := goldclient.GoldClientConfig{
- WorkDir: i.workDir,
InstanceID: i.instanceID,
+ OverrideBucket: i.bucketOverride,
OverrideGoldURL: i.urlOverride,
+ WorkDir: i.workDir,
}
goldClient, err = goldclient.NewCloudClient(config)
ifErrLogExit(ctx, err)
@@ -250,6 +254,7 @@
config := goldclient.GoldClientConfig{
FailureFile: i.failureFile,
InstanceID: i.instanceID,
+ OverrideBucket: i.bucketOverride,
OverrideGoldURL: i.urlOverride,
PassFailStep: i.passFailStep,
UploadOnly: i.uploadOnly,
@@ -317,6 +322,7 @@
config := goldclient.GoldClientConfig{
FailureFile: i.failureFile,
InstanceID: i.instanceID,
+ OverrideBucket: i.bucketOverride,
OverrideGoldURL: i.urlOverride,
PassFailStep: i.passFailStep,
UploadOnly: i.uploadOnly,
diff --git a/gold-client/cmd/goldctl/cmd_imgtest_test.go b/gold-client/cmd/goldctl/cmd_imgtest_test.go
index 7ac6ed5..77bd8c0 100644
--- a/gold-client/cmd/goldctl/cmd_imgtest_test.go
+++ b/gold-client/cmd/goldctl/cmd_imgtest_test.go
@@ -56,7 +56,7 @@
keysFile := filepath.Join(workDir, "keys.json")
require.NoError(t, ioutil.WriteFile(keysFile, []byte(`{"os": "Android"}`), 0644))
- mh := mockRPCResponses().Positive("pixel-tests", blankDigest).
+ mh := mockRPCResponses("https://my-instance-gold.skia.org").Positive("pixel-tests", blankDigest).
Negative("other-test", blankDigest).
Known("11111111111111111111111111111111").Build()
@@ -92,7 +92,7 @@
td, err := testutils.TestDataDir()
require.NoError(t, err)
- mh := mockRPCResponses().Build()
+ mh := mockRPCResponses("https://my-instance-gold.skia.org").Build()
// Call imgtest init with the following flags. We expect it to load the baseline expectations
// and the known hashes (both empty).
@@ -161,6 +161,85 @@
assert.Contains(t, string(fb), "https://my-instance-gold.skia.org/detail?test=pixel-tests&digest=00000000000000000000000000000000")
}
+func TestImgTest_InitAdd_OverwriteBucketAndURL_ProperLinks(t *testing.T) {
+ unittest.MediumTest(t)
+
+ workDir := t.TempDir()
+ setupAuthWithGSUtil(t, workDir)
+ td, err := testutils.TestDataDir()
+ require.NoError(t, err)
+
+ mh := mockRPCResponses("https://my-custom-gold-url.example.com").Build()
+
+ // Call imgtest init with the following flags. We expect it to load the baseline expectations
+ // and the known hashes (both empty).
+ ctx, output, exit := testContext(nil, mh, nil, nil)
+ env := imgTest{
+ bucketOverride: "my-custom-bucket",
+ commitHash: "1234567890123456789012345678901234567890",
+ corpus: "my_corpus",
+ instanceID: "my-instance",
+ passFailStep: true,
+ failureFile: filepath.Join(workDir, "failures.txt"),
+ workDir: workDir,
+ testKeysStrings: []string{"os:Android"},
+ urlOverride: "https://my-custom-gold-url.example.com",
+ }
+ runUntilExit(t, func() {
+ env.Init(ctx)
+ })
+ exit.AssertWasCalledWithCode(t, 0, output.String())
+
+ mg := &mocks.GCSUploader{}
+ resultsMatcher := mock.MatchedBy(func(results jsonio.GoldResults) bool {
+ assert.Equal(t, jsonio.GoldResults{
+ GitHash: "1234567890123456789012345678901234567890",
+ Key: map[string]string{
+ "os": "Android",
+ "source_type": "my_corpus",
+ },
+ Results: []*jsonio.Result{{
+ Key: map[string]string{"name": "pixel-tests", "device": "angler"},
+ Options: map[string]string{"some_option": "is optional", "ext": "png"},
+ Digest: blankDigest,
+ }},
+ }, results)
+ return true
+ })
+ mg.On("UploadJSON", testutils.AnyContext, resultsMatcher, mock.Anything,
+ `my-custom-bucket/dm-json-v1/2021/01/23/22/1234567890123456789012345678901234567890/waterfall/dm-1611440480000000019.json`).Return(nil)
+ bytesMatcher := mock.MatchedBy(func(b []byte) bool {
+ assert.Len(t, b, 78) // spot check length
+ return true
+ })
+ mg.On("UploadBytes", testutils.AnyContext, bytesMatcher, mock.Anything,
+ `gs://my-custom-bucket/dm-images-v1/00000000000000000000000000000000.png`).Return(nil)
+
+ // Now call imgtest add with the following flags. This is simulating a test uploading a single
+ // result for a test called pixel-tests.
+ ctx, output, exit = testContext(mg, nil, nil, mockTime(timeOne))
+ env = imgTest{
+ workDir: workDir,
+ testName: "pixel-tests",
+ pngFile: filepath.Join(td, "00000000000000000000000000000000.png"),
+ pngDigest: blankDigest,
+ testKeysStrings: []string{"device:angler"},
+ testOptionalKeysStrings: []string{"some_option:is optional"},
+ }
+ runUntilExit(t, func() {
+ env.Add(ctx)
+ })
+ logs := output.String()
+ exit.AssertWasCalledWithCode(t, 1, logs)
+ mg.AssertExpectations(t)
+ assert.Contains(t, logs, `Untriaged or negative image: https://my-custom-gold-url.example.com/detail?test=pixel-tests&digest=00000000000000000000000000000000`)
+ assert.Contains(t, logs, `Test: pixel-tests FAIL`)
+
+ fb, err := ioutil.ReadFile(filepath.Join(workDir, "failures.txt"))
+ require.NoError(t, err)
+ assert.Contains(t, string(fb), "https://my-custom-gold-url.example.com/detail?test=pixel-tests&digest=00000000000000000000000000000000")
+}
+
func TestImgTest_InitAdd_StreamingPassFail_MatchesExpectations_ZeroExitCode(t *testing.T) {
unittest.MediumTest(t)
@@ -169,7 +248,7 @@
td, err := testutils.TestDataDir()
require.NoError(t, err)
- mh := mockRPCResponses().Positive("pixel-tests", blankDigest).Build()
+ mh := mockRPCResponses("https://my-instance-gold.skia.org").Positive("pixel-tests", blankDigest).Build()
// Call imgtest init with the following flags. We expect it to load the baseline expectations
// and the known hashes.
@@ -238,7 +317,7 @@
td, err := testutils.TestDataDir()
require.NoError(t, err)
- mh := mockRPCResponses().Positive("pixel-tests", blankDigest).Build()
+ mh := mockRPCResponses("https://my-instance-gold.skia.org").Positive("pixel-tests", blankDigest).Build()
// Call imgtest init with the following flags. We expect it to load the baseline expectations
// and the known hashes.
@@ -342,7 +421,7 @@
keysFile := filepath.Join(workDir, "keys.json")
require.NoError(t, ioutil.WriteFile(keysFile, []byte(`{"os": "Android"}`), 0644))
- mh := mockRPCResponses().Positive("pixel-tests", blankDigest).Build()
+ mh := mockRPCResponses("https://my-instance-gold.skia.org").Positive("pixel-tests", blankDigest).Build()
mg := &mocks.GCSUploader{}
resultsMatcher := mock.MatchedBy(func(results jsonio.GoldResults) bool {
@@ -399,7 +478,7 @@
td, err := testutils.TestDataDir()
require.NoError(t, err)
- mh := mockRPCResponses().Positive("pixel-tests", blankDigest).Build()
+ mh := mockRPCResponses("https://my-instance-gold.skia.org").Positive("pixel-tests", blankDigest).Build()
// Call imgtest init with the following flags. We expect it to load the baseline expectations
// and the known hashes.
@@ -493,7 +572,7 @@
td, err := testutils.TestDataDir()
require.NoError(t, err)
- mh := mockRPCResponses().Build()
+ mh := mockRPCResponses("https://my-instance-gold.skia.org").Build()
// Call imgtest init with the following flags. We expect it to load the baseline expectations
// and the known hashes.
@@ -599,7 +678,7 @@
td, err := testutils.TestDataDir()
require.NoError(t, err)
- mh := mockRPCResponses().Positive("pixel-tests", a01Digest).
+ mh := mockRPCResponses("https://my-instance-gold.skia.org").Positive("pixel-tests", a01Digest).
LatestPositive(a01Digest, paramtools.Params{
"device": "bullhead", "name": "pixel-tests", "source_type": "my-instance",
}).Build()
@@ -639,7 +718,7 @@
td, err := testutils.TestDataDir()
require.NoError(t, err)
- mh := mockRPCResponses().Positive("pixel-tests", a01Digest).
+ mh := mockRPCResponses("https://my-instance-gold.skia.org").Positive("pixel-tests", a01Digest).
LatestPositive(a01Digest, paramtools.Params{
"device": "bullhead", "name": "pixel-tests", "source_type": "my-instance",
}).BuildForCL("gerritHub", "cl_1234")
@@ -709,11 +788,13 @@
knownDigests []string
exp *expectations.Expectations
latestPositives map[tiling.TraceID]types.Digest
+ urlBase string
}
-func mockRPCResponses() *rpcResponsesBuilder {
+func mockRPCResponses(instanceURL string) *rpcResponsesBuilder {
return &rpcResponsesBuilder{
- exp: &expectations.Expectations{},
+ exp: &expectations.Expectations{},
+ urlBase: instanceURL,
}
}
@@ -746,7 +827,7 @@
func (r *rpcResponsesBuilder) Build() *mocks.HTTPClient {
mh := &mocks.HTTPClient{}
knownResp := strings.Join(r.knownDigests, "\n")
- mh.On("Get", "https://my-instance-gold.skia.org/json/v1/hashes").Return(httpResponse(knownResp, "200 OK", http.StatusOK), nil)
+ mh.On("Get", r.urlBase+"/json/v1/hashes").Return(httpResponse(knownResp, "200 OK", http.StatusOK), nil)
exp, err := json.Marshal(baseline.Baseline{
MD5: "somemd5",
@@ -755,7 +836,7 @@
if err != nil {
panic(err)
}
- mh.On("Get", "https://my-instance-gold.skia.org/json/v2/expectations").Return(
+ mh.On("Get", r.urlBase+"/json/v2/expectations").Return(
httpResponse(string(exp), "200 OK", http.StatusOK), nil)
for traceID, digest := range r.latestPositives {
@@ -763,7 +844,7 @@
if err != nil {
panic(err)
}
- url := string("https://my-instance-gold.skia.org/json/v1/latestpositivedigest/" + traceID)
+ url := r.urlBase + "/json/v1/latestpositivedigest/" + string(traceID)
mh.On("Get", url).Return(
httpResponse(string(j), "200 OK", http.StatusOK), nil)
}
@@ -774,7 +855,7 @@
func (r *rpcResponsesBuilder) BuildForCL(crs, clID string) *mocks.HTTPClient {
mh := &mocks.HTTPClient{}
knownResp := strings.Join(r.knownDigests, "\n")
- mh.On("Get", "https://my-instance-gold.skia.org/json/v1/hashes").Return(httpResponse(knownResp, "200 OK", http.StatusOK), nil)
+ mh.On("Get", r.urlBase+"/json/v1/hashes").Return(httpResponse(knownResp, "200 OK", http.StatusOK), nil)
exp, err := json.Marshal(baseline.Baseline{
MD5: "somemd5",
@@ -785,7 +866,7 @@
if err != nil {
panic(err)
}
- url := fmt.Sprintf("https://my-instance-gold.skia.org/json/v2/expectations?issue=%s&crs=%s", clID, crs)
+ url := fmt.Sprintf("%s/json/v2/expectations?issue=%s&crs=%s", r.urlBase, clID, crs)
mh.On("Get", url).Return(
httpResponse(string(exp), "200 OK", http.StatusOK), nil)
@@ -794,7 +875,7 @@
if err != nil {
panic(err)
}
- url := string("https://my-instance-gold.skia.org/json/v1/latestpositivedigest/" + traceID)
+ url := r.urlBase + "/json/v1/latestpositivedigest/" + string(traceID)
mh.On("Get", url).Return(
httpResponse(string(j), "200 OK", http.StatusOK), nil)
}
@@ -805,7 +886,7 @@
func TestRPCResponsesBuilder_Default_ReturnsBlankValues(t *testing.T) {
unittest.SmallTest(t)
- mh := mockRPCResponses().Build()
+ mh := mockRPCResponses("https://my-instance-gold.skia.org").Build()
resp, err := mh.Get("https://my-instance-gold.skia.org/json/v1/hashes")
require.NoError(t, err)
b, err := ioutil.ReadAll(resp.Body)
@@ -822,7 +903,7 @@
func TestRPCResponsesBuilder_WithValues_ReturnsValidListsAndJSON(t *testing.T) {
unittest.SmallTest(t)
- mh := mockRPCResponses().
+ mh := mockRPCResponses("http://my-custom-url.example.com").
Known("first_digest").
Positive("alpha test", "second_digest").
Positive("beta test", "third_digest").
@@ -830,7 +911,7 @@
LatestPositive("third_digest", paramtools.Params{"alpha": "beta", "gamma": "delta epsilon"}).
Known("fifth_digest").
Build()
- resp, err := mh.Get("https://my-instance-gold.skia.org/json/v1/hashes")
+ resp, err := mh.Get("http://my-custom-url.example.com/json/v1/hashes")
require.NoError(t, err)
b, err := ioutil.ReadAll(resp.Body)
require.NoError(t, err)
@@ -840,13 +921,13 @@
fourth_digest
fifth_digest`, string(b))
- resp, err = mh.Get("https://my-instance-gold.skia.org/json/v2/expectations")
+ resp, err = mh.Get("http://my-custom-url.example.com/json/v2/expectations")
require.NoError(t, err)
b, err = ioutil.ReadAll(resp.Body)
require.NoError(t, err)
assert.Equal(t, `{"md5":"somemd5","primary":{"alpha test":{"fourth_digest":"negative","second_digest":"positive"},"beta test":{"third_digest":"positive"}}}`, string(b))
- resp, err = mh.Get("https://my-instance-gold.skia.org/json/v1/latestpositivedigest/,alpha=beta,gamma=delta epsilon,")
+ resp, err = mh.Get("http://my-custom-url.example.com/json/v1/latestpositivedigest/,alpha=beta,gamma=delta epsilon,")
require.NoError(t, err)
b, err = ioutil.ReadAll(resp.Body)
require.NoError(t, err)
diff --git a/gold-client/go/goldclient/common.go b/gold-client/go/goldclient/common.go
index 9942e1b..fc09d65 100644
--- a/gold-client/go/goldclient/common.go
+++ b/gold-client/go/goldclient/common.go
@@ -17,18 +17,6 @@
const maxAttempts = 5
-// GetGoldInstanceURL returns the URL for a given Gold instance id.
-// This is usually a formulaic transform, but there are some special cases.
-func GetGoldInstanceURL(instanceID string) string {
- if instanceID == instanceIDSkiaLegacy {
- return hostSkiaLegacy
- }
- if instanceID == instanceIDFuchsia {
- return hostFuchsiaCorp
- }
- return fmt.Sprintf(goldHostTemplate, instanceID)
-}
-
// getWithRetries makes a GET request with retries to work around the rare unexpected EOF error.
// See https://crbug.com/skia/9108.
//
diff --git a/gold-client/go/goldclient/goldclient.go b/gold-client/go/goldclient/goldclient.go
index 81c2878..29fd9bd 100644
--- a/gold-client/go/goldclient/goldclient.go
+++ b/gold-client/go/goldclient/goldclient.go
@@ -160,9 +160,12 @@
// any failures. Only written to if PassFailStep is true
FailureFile string
- // OverrideGoldURL is optional and allows to override the GoldURL for testing.
+ // OverrideGoldURL is optional and overrides the formulaic GoldURL (typically legacy instances).
OverrideGoldURL string
+ // OverrideBucket is optional and overrides the formulaic GCS Bucket.
+ OverrideBucket string
+
// UploadOnly is a mode where we don't check expectations against the server - i.e.
// we just operate in upload mode.
UploadOnly bool
@@ -261,10 +264,11 @@
WorkDir: c.workDir,
}
if c.resultState != nil {
- existingConfig.InstanceID = c.resultState.InstanceID
- existingConfig.PassFailStep = c.resultState.PerTestPassFail
existingConfig.FailureFile = c.resultState.FailureFile
+ existingConfig.InstanceID = c.resultState.InstanceID
+ existingConfig.OverrideBucket = c.resultState.Bucket
existingConfig.OverrideGoldURL = c.resultState.GoldURL
+ existingConfig.PassFailStep = c.resultState.PerTestPassFail
existingConfig.UploadOnly = c.resultState.UploadOnly
}
c.resultState = newResultState(sharedConfig, &existingConfig)
diff --git a/gold-client/go/goldclient/resultstate.go b/gold-client/go/goldclient/resultstate.go
index 28856f6..90b98c6 100644
--- a/gold-client/go/goldclient/resultstate.go
+++ b/gold-client/go/goldclient/resultstate.go
@@ -30,13 +30,6 @@
// bucketTemplate constructs the name of the ingestion bucket from the instance id
bucketTemplate = "skia-gold-%s"
- // Skia's naming conventions are old and don't follow the patterns that
- // newer clients do. One day, it might be nice to align the skia names
- // to match the rest.
- bucketSkiaLegacy = "skia-infra-gm"
- hostSkiaLegacy = "https://gold.skia.org"
- instanceIDSkiaLegacy = "skia"
-
hostFuchsiaCorp = "https://fuchsia-gold.corp.goog"
instanceIDFuchsia = "fuchsia"
)
@@ -64,7 +57,11 @@
func newResultState(sharedConfig jsonio.GoldResults, config *GoldClientConfig) *resultState {
goldURL := config.OverrideGoldURL
if goldURL == "" {
- goldURL = GetGoldInstanceURL(config.InstanceID)
+ goldURL = getGoldInstanceURL(config.InstanceID)
+ }
+ bucket := config.OverrideBucket
+ if bucket == "" {
+ bucket = getBucket(config.InstanceID)
}
ret := &resultState{
@@ -74,18 +71,24 @@
InstanceID: config.InstanceID,
UploadOnly: config.UploadOnly,
GoldURL: goldURL,
- Bucket: getBucket(config.InstanceID),
+ Bucket: bucket,
}
return ret
}
-// getBucket returns the bucket name for a given instance id.
+// getGoldInstanceURL returns the URL for a given Gold instance id.
// This is usually a formulaic transform, but there are some special cases.
-func getBucket(instanceID string) string {
- if instanceID == instanceIDSkiaLegacy {
- return bucketSkiaLegacy
+func getGoldInstanceURL(instanceID string) string {
+ if instanceID == instanceIDFuchsia {
+ return hostFuchsiaCorp
}
+ return fmt.Sprintf(goldHostTemplate, instanceID)
+}
+
+// getBucket returns the formulaic bucket name for a given instance id. Legacy instances may
+// have a different naming scheme and would override the bucket.
+func getBucket(instanceID string) string {
return fmt.Sprintf(bucketTemplate, instanceID)
}
