From 528c9db37de43440e3dfbde4d4f928ed82bae152 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Fri, 20 Nov 2020 18:39:14 -0800 Subject: [PATCH 1/3] campaigns: allow env usage in campaign steps This is the src-cli component of sourcegraph/sourcegraph#15822. There's also a new feature flag in here to maintain backward compatibility with older Sourcegraph versions, at the cost of extra work when parsing the campaign spec. I think that's the right tradeoff. --- CHANGELOG.md | 2 + go.mod | 2 +- go.sum | 4 +- internal/campaigns/campaign_spec.go | 20 ++++- internal/campaigns/execution_cache.go | 27 +++++++ internal/campaigns/execution_cache_test.go | 89 ++++++++++++++++++++++ internal/campaigns/features.go | 2 + internal/campaigns/features_test.go | 1 + internal/campaigns/run_steps.go | 10 ++- internal/campaigns/service.go | 2 +- schema/campaign_spec.schema.json | 33 ++++++-- schema/campaign_spec_stringdata.go | 33 ++++++-- 12 files changed, 207 insertions(+), 18 deletions(-) create mode 100644 internal/campaigns/execution_cache_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e1cdcf6591..9c8e0f02e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ All notable changes to `src-cli` are documented in this file. ### Added +- Campaign steps may now include environment variables from outside of the campaign spec using [array syntax](http://docs.sourcegraph.com/campaigns/references/campaign_spec_yaml_reference#environment-array). [#392](https://github.com/sourcegraph/src-cli/pull/392) + ### Changed ### Fixed diff --git a/go.mod b/go.mod index f7ea42ae16..d7407d9cc8 100644 --- a/go.mod +++ b/go.mod @@ -19,7 +19,7 @@ require ( github.com/olekukonko/tablewriter v0.0.4 // indirect github.com/pkg/browser v0.0.0-20180916011732-0a3d74bf9ce4 github.com/pkg/errors v0.9.1 - github.com/sourcegraph/campaignutils v0.0.0-20201016010611-63eb2bca27ad + github.com/sourcegraph/campaignutils v0.0.0-20201123212545-3f96e1fcd9b3 github.com/sourcegraph/codeintelutils v0.0.0-20201118031531-b82ba3167b30 github.com/sourcegraph/go-diff v0.6.1 github.com/sourcegraph/jsonx v0.0.0-20200629203448-1a936bd500cf diff --git a/go.sum b/go.sum index 560a3a99ba..9ca9c30ff8 100644 --- a/go.sum +++ b/go.sum @@ -50,8 +50,8 @@ github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZb github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/shurcooL/go v0.0.0-20180423040247-9e1955d9fb6e/go.mod h1:TDJrrUr11Vxrven61rcy3hJMUqaf/CLWYhHNPmT14Lk= github.com/shurcooL/go-goon v0.0.0-20170922171312-37c2f522c041/go.mod h1:N5mDOmsrJOB+vfqUK+7DmDyjhSLIIBnXo9lvZJj3MWQ= -github.com/sourcegraph/campaignutils v0.0.0-20201016010611-63eb2bca27ad h1:HeSWFpxau4Jqk0s4yEhOdep+KYrJDm0497uhb/hnsgU= -github.com/sourcegraph/campaignutils v0.0.0-20201016010611-63eb2bca27ad/go.mod h1:xm6i78Mk2t4DBLQDqEFc/3x6IPf7yYZCgbNaTQGhJHA= +github.com/sourcegraph/campaignutils v0.0.0-20201123212545-3f96e1fcd9b3 h1:JYGPqpRHJ5fWUFRomZ4Zgvn5N15hrdQDhY03Pp1g7Wc= +github.com/sourcegraph/campaignutils v0.0.0-20201123212545-3f96e1fcd9b3/go.mod h1:xm6i78Mk2t4DBLQDqEFc/3x6IPf7yYZCgbNaTQGhJHA= github.com/sourcegraph/codeintelutils v0.0.0-20201118031531-b82ba3167b30 h1:HrRrPyskdkHc6MqQS3ehH+DSraFnOhtvBWQ6AzEJC1o= github.com/sourcegraph/codeintelutils v0.0.0-20201118031531-b82ba3167b30/go.mod h1:HplI8gRslTrTUUsSYwu28hSOderix7m5dHNca7xBzeo= github.com/sourcegraph/go-diff v0.6.1 h1:hmA1LzxW0n1c3Q4YbrFgg4P99GSnebYa3x8gr0HZqLQ= diff --git a/internal/campaigns/campaign_spec.go b/internal/campaigns/campaign_spec.go index 2ba4fff08d..eb68c2d85a 100644 --- a/internal/campaigns/campaign_spec.go +++ b/internal/campaigns/campaign_spec.go @@ -3,6 +3,9 @@ package campaigns import ( "fmt" + "github.com/hashicorp/go-multierror" + "github.com/pkg/errors" + "github.com/sourcegraph/campaignutils/env" "github.com/sourcegraph/campaignutils/overridable" "github.com/sourcegraph/campaignutils/yaml" "github.com/sourcegraph/src-cli/schema" @@ -65,18 +68,31 @@ type OnQueryOrRepository struct { type Step struct { Run string `json:"run,omitempty" yaml:"run"` Container string `json:"container,omitempty" yaml:"container"` - Env map[string]string `json:"env,omitempty" yaml:"env"` + Env env.Environment `json:"env,omitempty" yaml:"env"` Files map[string]string `json:"files,omitempty" yaml:"files,omitempty"` image string } -func ParseCampaignSpec(data []byte) (*CampaignSpec, error) { +func ParseCampaignSpec(data []byte, features featureFlags) (*CampaignSpec, error) { var spec CampaignSpec if err := yaml.UnmarshalValidate(schema.CampaignSpecJSON, data, &spec); err != nil { return nil, err } + if !features.allowArrayEnvironments { + var errs *multierror.Error + for i, step := range spec.Steps { + if !step.Env.IsStatic() { + errs = multierror.Append(errs, errors.Errorf("step %d includes one or more dynamic environment variables, which are unsupported in this Sourcegraph version", i+1)) + } + } + + if err := errs.ErrorOrNil(); err != nil { + return nil, err + } + } + return &spec, nil } diff --git a/internal/campaigns/execution_cache.go b/internal/campaigns/execution_cache.go index 380d9b8f48..b29a0b3b8f 100644 --- a/internal/campaigns/execution_cache.go +++ b/internal/campaigns/execution_cache.go @@ -24,6 +24,33 @@ type ExecutionCacheKey struct { *Task } +func (key ExecutionCacheKey) MarshalJSON() ([]byte, error) { + // We have to resolve the step environments and include them in the cache + // key to ensure that the cache is properly invalidated when an environment + // variable changes. + // + // Note that we don't base the cache key on the entire global environment: + // if an unrelated environment variable changes, that's fine. We're only + // interested in the ones that actually make it into the step container. + global := os.Environ() + envs := make([]map[string]string, len(key.Task.Steps)) + for i, step := range key.Task.Steps { + env, err := step.Env.Resolve(global) + if err != nil { + return nil, errors.Wrapf(err, "resolving environment for step %d", i) + } + envs[i] = env + } + + return json.Marshal(struct { + *Task + Environments []map[string]string + }{ + Task: key.Task, + Environments: envs, + }) +} + type ExecutionCache interface { Get(ctx context.Context, key ExecutionCacheKey) (result *ChangesetSpec, err error) Set(ctx context.Context, key ExecutionCacheKey, result *ChangesetSpec) error diff --git a/internal/campaigns/execution_cache_test.go b/internal/campaigns/execution_cache_test.go new file mode 100644 index 0000000000..6b1016178f --- /dev/null +++ b/internal/campaigns/execution_cache_test.go @@ -0,0 +1,89 @@ +package campaigns + +import ( + "encoding/json" + "os" + "testing" + + "gopkg.in/yaml.v2" +) + +const testExecutionCacheKeyEnv = "TEST_EXECUTION_CACHE_KEY_ENV" + +func TestExecutionCacheKey(t *testing.T) { + // Let's set up an array of steps that we can test with. One step will + // depend on an environment variable outside the spec. + var steps []Step + if err := yaml.Unmarshal([]byte(` +- run: foo + env: + FOO: BAR + +- run: bar + env: + - FOO: BAR + - `+testExecutionCacheKeyEnv+` +`), &steps); err != nil { + t.Fatal(err) + } + + // And now we can set up a key to work with. + key := ExecutionCacheKey{&Task{Steps: steps}} + + // All righty. Let's get ourselves a baseline cache key here. + initial, err := json.Marshal(key) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + // Let's set an unrelated environment variable and ensure we still have the + // same key. + if err := os.Setenv(testExecutionCacheKeyEnv+"_UNRELATED", "foo"); err != nil { + t.Fatal(err) + } + have, err := json.Marshal(key) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if string(initial) != string(have) { + t.Errorf("unexpected change in key: initial=%q have=%q", initial, have) + } + + // Let's now set the environment variable referenced in the steps and verify + // that the cache key does change. + if err := os.Setenv(testExecutionCacheKeyEnv, "foo"); err != nil { + t.Fatal(err) + } + have, err = json.Marshal(key) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if string(initial) == string(have) { + t.Errorf("unexpected lack of change in key: %q", have) + } + + // And, just to be sure, let's change it again. + if err := os.Setenv(testExecutionCacheKeyEnv, "bar"); err != nil { + t.Fatal(err) + } + again, err := json.Marshal(key) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if string(initial) == string(again) || string(have) == string(again) { + t.Errorf("unexpected lack of change in key: %q", again) + } + + // Finally, if we unset the environment variable again, we should get a key + // that matches the initial key. + if err := os.Unsetenv(testExecutionCacheKeyEnv); err != nil { + t.Fatal(err) + } + have, err = json.Marshal(key) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if string(initial) != string(have) { + t.Errorf("unexpected change in key: initial=%q have=%q", initial, have) + } +} diff --git a/internal/campaigns/features.go b/internal/campaigns/features.go index bf44a59c1a..7597148d45 100644 --- a/internal/campaigns/features.go +++ b/internal/campaigns/features.go @@ -8,6 +8,7 @@ import ( // featureFlags represent features that are only available on certain // Sourcegraph versions and we therefore have to detect at runtime. type featureFlags struct { + allowArrayEnvironments bool includeAutoAuthorDetails bool useGzipCompression bool } @@ -18,6 +19,7 @@ func (ff *featureFlags) setFromVersion(version string) error { constraint string minDate string }{ + {&ff.allowArrayEnvironments, ">= 3.23.0", "2020-11-24"}, {&ff.includeAutoAuthorDetails, ">= 3.20.0", "2020-09-10"}, {&ff.useGzipCompression, ">= 3.21.0", "2020-10-12"}, } { diff --git a/internal/campaigns/features_test.go b/internal/campaigns/features_test.go index 82f67706af..1f997812eb 100644 --- a/internal/campaigns/features_test.go +++ b/internal/campaigns/features_test.go @@ -2,6 +2,7 @@ package campaigns func featuresAllEnabled() featureFlags { return featureFlags{ + allowArrayEnvironments: true, includeAutoAuthorDetails: true, useGzipCompression: true, } diff --git a/internal/campaigns/run_steps.go b/internal/campaigns/run_steps.go index 79aa1ac432..e53153cf21 100644 --- a/internal/campaigns/run_steps.go +++ b/internal/campaigns/run_steps.go @@ -168,10 +168,16 @@ func runSteps(ctx context.Context, wc *WorkspaceCreator, repo *graphql.Repositor filesToMount[name] = fp } + // Resolve step.Env given the current environment. + stepEnv, err := step.Env.Resolve(os.Environ()) + if err != nil { + return nil, errors.Wrap(err, "resolving step environment") + } + // Render the step.Env variables as templates. - env, err := renderStepEnv(step.Env, &stepContext) + env, err := renderStepEnv(stepEnv, &stepContext) if err != nil { - return nil, errors.Wrap(err, "parsing step files") + return nil, errors.Wrap(err, "parsing step environment") } reportProgress(runScript.String()) diff --git a/internal/campaigns/service.go b/internal/campaigns/service.go index 5b285da17b..c6f532d1dc 100644 --- a/internal/campaigns/service.go +++ b/internal/campaigns/service.go @@ -293,7 +293,7 @@ func (svc *Service) ParseCampaignSpec(in io.Reader) (*CampaignSpec, string, erro return nil, "", errors.Wrap(err, "reading campaign spec") } - spec, err := ParseCampaignSpec(data) + spec, err := ParseCampaignSpec(data, svc.features) if err != nil { return nil, "", errors.Wrap(err, "parsing campaign spec") } diff --git a/schema/campaign_spec.schema.json b/schema/campaign_spec.schema.json index 985af68a43..4dec085fff 100644 --- a/schema/campaign_spec.schema.json +++ b/schema/campaign_spec.schema.json @@ -76,11 +76,34 @@ "examples": ["alpine:3"] }, "env": { - "type": "object", - "description": "Environment variables to set in the environment when running this command.", - "additionalProperties": { - "type": "string" - } + "description": "Environment variables to set in the step environment.", + "oneOf": [ + { + "type": "object", + "description": "Environment variables to set in the step environment.", + "additionalProperties": { + "type": "string" + } + }, + { + "type": "array", + "items": { + "oneOf": [ + { + "type": "string", + "description": "An environment variable to set in the step environment: the value will be passed through from the environment src is running within." + }, + { + "type": "object", + "description": "An environment variable to set in the step environment: the value will be passed through from the environment src is running within.", + "additionalProperties": { "type": "string" }, + "minProperties": 1, + "maxProperties": 1 + } + ] + } + } + ] }, "files": { "type": "object", diff --git a/schema/campaign_spec_stringdata.go b/schema/campaign_spec_stringdata.go index e4e05dc641..2432053a88 100644 --- a/schema/campaign_spec_stringdata.go +++ b/schema/campaign_spec_stringdata.go @@ -81,11 +81,34 @@ const CampaignSpecJSON = `{ "examples": ["alpine:3"] }, "env": { - "type": "object", - "description": "Environment variables to set in the environment when running this command.", - "additionalProperties": { - "type": "string" - } + "description": "Environment variables to set in the step environment.", + "oneOf": [ + { + "type": "object", + "description": "Environment variables to set in the step environment.", + "additionalProperties": { + "type": "string" + } + }, + { + "type": "array", + "items": { + "oneOf": [ + { + "type": "string", + "description": "An environment variable to set in the step environment: the value will be passed through from the environment src is running within." + }, + { + "type": "object", + "description": "An environment variable to set in the step environment: the value will be passed through from the environment src is running within.", + "additionalProperties": { "type": "string" }, + "minProperties": 1, + "maxProperties": 1 + } + ] + } + } + ] }, "files": { "type": "object", From 3ed24a6ee035ec0f3df0b8ce2abc50c61b04e492 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Mon, 23 Nov 2020 13:55:22 -0800 Subject: [PATCH 2/3] Hide implementation details of cache key generation. --- internal/campaigns/execution_cache.go | 21 +++++++++++++-------- internal/campaigns/execution_cache_test.go | 11 +++++------ 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/internal/campaigns/execution_cache.go b/internal/campaigns/execution_cache.go index b29a0b3b8f..af5e6610e2 100644 --- a/internal/campaigns/execution_cache.go +++ b/internal/campaigns/execution_cache.go @@ -24,7 +24,9 @@ type ExecutionCacheKey struct { *Task } -func (key ExecutionCacheKey) MarshalJSON() ([]byte, error) { +// Key converts the key into a string form that can be used to uniquely identify +// the cache key in a more concise form than the entire Task. +func (key ExecutionCacheKey) Key() (string, error) { // We have to resolve the step environments and include them in the cache // key to ensure that the cache is properly invalidated when an environment // variable changes. @@ -37,18 +39,24 @@ func (key ExecutionCacheKey) MarshalJSON() ([]byte, error) { for i, step := range key.Task.Steps { env, err := step.Env.Resolve(global) if err != nil { - return nil, errors.Wrapf(err, "resolving environment for step %d", i) + return "", errors.Wrapf(err, "resolving environment for step %d", i) } envs[i] = env } - return json.Marshal(struct { + raw, err := json.Marshal(struct { *Task Environments []map[string]string }{ Task: key.Task, Environments: envs, }) + if err != nil { + return "", err + } + + hash := sha256.Sum256(raw) + return base64.RawURLEncoding.EncodeToString(hash[:16]), nil } type ExecutionCache interface { @@ -62,14 +70,11 @@ type ExecutionDiskCache struct { } func (c ExecutionDiskCache) cacheFilePath(key ExecutionCacheKey) (string, error) { - keyJSON, err := json.Marshal(key) + keyString, err := key.Key() if err != nil { - return "", errors.Wrap(err, "Failed to marshal JSON when generating action cache key") + return "", errors.Wrap(err, "calculating execution cache key") } - b := sha256.Sum256(keyJSON) - keyString := base64.RawURLEncoding.EncodeToString(b[:16]) - return filepath.Join(c.Dir, keyString+".json"), nil } diff --git a/internal/campaigns/execution_cache_test.go b/internal/campaigns/execution_cache_test.go index 6b1016178f..8ab99fa7a6 100644 --- a/internal/campaigns/execution_cache_test.go +++ b/internal/campaigns/execution_cache_test.go @@ -1,7 +1,6 @@ package campaigns import ( - "encoding/json" "os" "testing" @@ -31,7 +30,7 @@ func TestExecutionCacheKey(t *testing.T) { key := ExecutionCacheKey{&Task{Steps: steps}} // All righty. Let's get ourselves a baseline cache key here. - initial, err := json.Marshal(key) + initial, err := key.Key() if err != nil { t.Errorf("unexpected error: %v", err) } @@ -41,7 +40,7 @@ func TestExecutionCacheKey(t *testing.T) { if err := os.Setenv(testExecutionCacheKeyEnv+"_UNRELATED", "foo"); err != nil { t.Fatal(err) } - have, err := json.Marshal(key) + have, err := key.Key() if err != nil { t.Errorf("unexpected error: %v", err) } @@ -54,7 +53,7 @@ func TestExecutionCacheKey(t *testing.T) { if err := os.Setenv(testExecutionCacheKeyEnv, "foo"); err != nil { t.Fatal(err) } - have, err = json.Marshal(key) + have, err = key.Key() if err != nil { t.Errorf("unexpected error: %v", err) } @@ -66,7 +65,7 @@ func TestExecutionCacheKey(t *testing.T) { if err := os.Setenv(testExecutionCacheKeyEnv, "bar"); err != nil { t.Fatal(err) } - again, err := json.Marshal(key) + again, err := key.Key() if err != nil { t.Errorf("unexpected error: %v", err) } @@ -79,7 +78,7 @@ func TestExecutionCacheKey(t *testing.T) { if err := os.Unsetenv(testExecutionCacheKeyEnv); err != nil { t.Fatal(err) } - have, err = json.Marshal(key) + have, err = key.Key() if err != nil { t.Errorf("unexpected error: %v", err) } From 66e71e41ffb4001cae8ad80d2c313c9870dfec34 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Mon, 23 Nov 2020 21:59:09 -0800 Subject: [PATCH 3/3] Bump after merging sourcegraph/campaignutils#6. --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index d7407d9cc8..ff2888e602 100644 --- a/go.mod +++ b/go.mod @@ -19,7 +19,7 @@ require ( github.com/olekukonko/tablewriter v0.0.4 // indirect github.com/pkg/browser v0.0.0-20180916011732-0a3d74bf9ce4 github.com/pkg/errors v0.9.1 - github.com/sourcegraph/campaignutils v0.0.0-20201123212545-3f96e1fcd9b3 + github.com/sourcegraph/campaignutils v0.0.0-20201124055807-2f9cfa9317e2 github.com/sourcegraph/codeintelutils v0.0.0-20201118031531-b82ba3167b30 github.com/sourcegraph/go-diff v0.6.1 github.com/sourcegraph/jsonx v0.0.0-20200629203448-1a936bd500cf diff --git a/go.sum b/go.sum index 9ca9c30ff8..3facdd1ef3 100644 --- a/go.sum +++ b/go.sum @@ -50,8 +50,8 @@ github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZb github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/shurcooL/go v0.0.0-20180423040247-9e1955d9fb6e/go.mod h1:TDJrrUr11Vxrven61rcy3hJMUqaf/CLWYhHNPmT14Lk= github.com/shurcooL/go-goon v0.0.0-20170922171312-37c2f522c041/go.mod h1:N5mDOmsrJOB+vfqUK+7DmDyjhSLIIBnXo9lvZJj3MWQ= -github.com/sourcegraph/campaignutils v0.0.0-20201123212545-3f96e1fcd9b3 h1:JYGPqpRHJ5fWUFRomZ4Zgvn5N15hrdQDhY03Pp1g7Wc= -github.com/sourcegraph/campaignutils v0.0.0-20201123212545-3f96e1fcd9b3/go.mod h1:xm6i78Mk2t4DBLQDqEFc/3x6IPf7yYZCgbNaTQGhJHA= +github.com/sourcegraph/campaignutils v0.0.0-20201124055807-2f9cfa9317e2 h1:MJu/6WzWdPegzYnZLb04IS0u4VyUpPIAHQyWT5i2vR8= +github.com/sourcegraph/campaignutils v0.0.0-20201124055807-2f9cfa9317e2/go.mod h1:xm6i78Mk2t4DBLQDqEFc/3x6IPf7yYZCgbNaTQGhJHA= github.com/sourcegraph/codeintelutils v0.0.0-20201118031531-b82ba3167b30 h1:HrRrPyskdkHc6MqQS3ehH+DSraFnOhtvBWQ6AzEJC1o= github.com/sourcegraph/codeintelutils v0.0.0-20201118031531-b82ba3167b30/go.mod h1:HplI8gRslTrTUUsSYwu28hSOderix7m5dHNca7xBzeo= github.com/sourcegraph/go-diff v0.6.1 h1:hmA1LzxW0n1c3Q4YbrFgg4P99GSnebYa3x8gr0HZqLQ=