From e4e52327fa361edc258a856b1cd5e8774b6df13f Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 5 Nov 2020 11:37:44 -0800 Subject: [PATCH] campaigns: restore pre-3.20 instance compatibility The author name and e-mail fields were always sent, but the schema Sourcegraph 3.19 uses to validate changeset specs don't include these fields. Since we're doing version detection for feature flags (quirks) now anyway, let's use that to not set the author name and e-mail if they're not explicitly provided when talking to a 3.19 instance. If the user provides an author name and/or e-mail and sends a changeset spec to a pre-3.20 instance, the request will fail, but since those are 3.20 features that's OK. --- CHANGELOG.md | 2 ++ internal/campaigns/changeset_spec.go | 4 ++-- internal/campaigns/executor.go | 30 +++++++++++++++----------- internal/campaigns/executor_test.go | 2 +- internal/campaigns/features.go | 32 ++++++++++++++++++++++++++++ internal/campaigns/features_test.go | 8 +++++++ internal/campaigns/service.go | 18 ++++++---------- 7 files changed, 68 insertions(+), 28 deletions(-) create mode 100644 internal/campaigns/features.go create mode 100644 internal/campaigns/features_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e8d1c0c1d8..e81d2903a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,8 @@ All notable changes to `src-cli` are documented in this file. ### Fixed +- Restored backward compatibility when creating campaigns against Sourcegraph 3.19, provided author details are not provided in the campaign spec. [#370](https://github.com/sourcegraph/src-cli/pull/370) + ### Removed ## 3.21.6 diff --git a/internal/campaigns/changeset_spec.go b/internal/campaigns/changeset_spec.go index b86f56e652..e0972e7f95 100644 --- a/internal/campaigns/changeset_spec.go +++ b/internal/campaigns/changeset_spec.go @@ -28,6 +28,6 @@ type CreatedChangeset struct { type GitCommitDescription struct { Message string `json:"message"` Diff string `json:"diff"` - AuthorName string `json:"authorName"` - AuthorEmail string `json:"authorEmail"` + AuthorName string `json:"authorName,omitempty"` + AuthorEmail string `json:"authorEmail,omitempty"` } diff --git a/internal/campaigns/executor.go b/internal/campaigns/executor.go index 60d7924102..07fb1e4294 100644 --- a/internal/campaigns/executor.go +++ b/internal/campaigns/executor.go @@ -85,12 +85,13 @@ func (ts *TaskStatus) IsCompleted() bool { type executor struct { ExecutorOpts - cache ExecutionCache - client api.Client - logger *LogManager - creator *WorkspaceCreator - tasks sync.Map - tempDir string + cache ExecutionCache + client api.Client + features featureFlags + logger *LogManager + creator *WorkspaceCreator + tasks sync.Map + tempDir string par *parallel.Run doneEnqueuing chan struct{} @@ -99,12 +100,13 @@ type executor struct { specsMu sync.Mutex } -func newExecutor(opts ExecutorOpts, client api.Client) *executor { +func newExecutor(opts ExecutorOpts, client api.Client, features featureFlags) *executor { return &executor{ ExecutorOpts: opts, cache: opts.Cache, creator: opts.Creator, client: client, + features: features, doneEnqueuing: make(chan struct{}), logger: NewLogManager(opts.TempDir, opts.KeepLogs), tempDir: opts.TempDir, @@ -208,7 +210,7 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { diff = result.Commits[0].Diff } - spec := createChangesetSpec(task, diff) + spec := createChangesetSpec(task, diff, x.features) status.Cached = true status.ChangesetSpec = spec @@ -267,7 +269,7 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { } // Build the changeset spec. - spec := createChangesetSpec(task, string(diff)) + spec := createChangesetSpec(task, string(diff), x.features) status.ChangesetSpec = spec x.updateTaskStatus(task, status) @@ -305,16 +307,18 @@ func reachedTimeout(cmdCtx context.Context, err error) bool { return errors.Is(err, context.DeadlineExceeded) } -func createChangesetSpec(task *Task, diff string) *ChangesetSpec { +func createChangesetSpec(task *Task, diff string, features featureFlags) *ChangesetSpec { repo := task.Repository.Name var authorName string var authorEmail string if task.Template.Commit.Author == nil { - // user did not provide author info, so use defaults - authorName = "Sourcegraph" - authorEmail = "campaigns@sourcegraph.com" + if features.includeAutoAuthorDetails { + // user did not provide author info, so use defaults + authorName = "Sourcegraph" + authorEmail = "campaigns@sourcegraph.com" + } } else { authorName = task.Template.Commit.Author.Name authorEmail = task.Template.Commit.Author.Email diff --git a/internal/campaigns/executor_test.go b/internal/campaigns/executor_test.go index 6cdd3d1782..8e4817cc5e 100644 --- a/internal/campaigns/executor_test.go +++ b/internal/campaigns/executor_test.go @@ -144,7 +144,7 @@ func TestExecutor_Integration(t *testing.T) { opts.Timeout = 5 * time.Second } - executor := newExecutor(opts, client) + executor := newExecutor(opts, client, featuresAllEnabled()) template := &ChangesetTemplate{} for _, r := range tc.repos { diff --git a/internal/campaigns/features.go b/internal/campaigns/features.go new file mode 100644 index 0000000000..bf44a59c1a --- /dev/null +++ b/internal/campaigns/features.go @@ -0,0 +1,32 @@ +package campaigns + +import ( + "github.com/pkg/errors" + "github.com/sourcegraph/src-cli/internal/api" +) + +// featureFlags represent features that are only available on certain +// Sourcegraph versions and we therefore have to detect at runtime. +type featureFlags struct { + includeAutoAuthorDetails bool + useGzipCompression bool +} + +func (ff *featureFlags) setFromVersion(version string) error { + for _, feature := range []struct { + flag *bool + constraint string + minDate string + }{ + {&ff.includeAutoAuthorDetails, ">= 3.20.0", "2020-09-10"}, + {&ff.useGzipCompression, ">= 3.21.0", "2020-10-12"}, + } { + value, err := api.CheckSourcegraphVersion(version, feature.constraint, feature.minDate) + if err != nil { + return errors.Wrap(err, "failed to check version returned by Sourcegraph") + } + *feature.flag = value + } + + return nil +} diff --git a/internal/campaigns/features_test.go b/internal/campaigns/features_test.go new file mode 100644 index 0000000000..82f67706af --- /dev/null +++ b/internal/campaigns/features_test.go @@ -0,0 +1,8 @@ +package campaigns + +func featuresAllEnabled() featureFlags { + return featureFlags{ + includeAutoAuthorDetails: true, + useGzipCompression: true, + } +} diff --git a/internal/campaigns/service.go b/internal/campaigns/service.go index 1a803f5f49..b932068f0f 100644 --- a/internal/campaigns/service.go +++ b/internal/campaigns/service.go @@ -17,9 +17,9 @@ import ( ) type Service struct { - allowUnsupported bool - client api.Client - useGzipCompression bool + allowUnsupported bool + client api.Client + features featureFlags } type ServiceOpts struct { @@ -71,17 +71,11 @@ func (svc *Service) DetermineFeatureFlags(ctx context.Context) error { return errors.Wrap(err, "failed to query Sourcegraph version to check for available features") } - supportsGzip, err := api.CheckSourcegraphVersion(version, ">= 3.21.0", "2020-10-12") - if err != nil { - return errors.Wrap(err, "failed to check version returned by Sourcegraph") - } - svc.useGzipCompression = supportsGzip - - return nil + return svc.features.setFromVersion(version) } func (svc *Service) newRequest(query string, vars map[string]interface{}) api.Request { - if svc.useGzipCompression { + if svc.features.useGzipCompression { return svc.client.NewGzippedRequest(query, vars) } return svc.client.NewRequest(query, vars) @@ -201,7 +195,7 @@ type ExecutorOpts struct { } func (svc *Service) NewExecutor(opts ExecutorOpts) Executor { - return newExecutor(opts, svc.client) + return newExecutor(opts, svc.client, svc.features) } func (svc *Service) NewWorkspaceCreator(dir string, cleanArchives bool) *WorkspaceCreator {