batches: make published field optional (#538)

The first part of sourcegraph/sourcegraph#18277.

Author: LawnGnome

CHANGELOG.md

@@ -13,6 +13,16 @@ All notable changes to `src-cli` are documented in this file.
 
 ### Added
 
+- Starting with Sourcegraph 3.30.0, the `published` field is optional in batch specs. If omitted, the publication state will be controlled through the Batch Changes UI. [#538](https://github.com/sourcegraph/src-cli/pull/538)
+
+### Changed
+
+### Fixed
+
+## 3.29.1
+
+### Added
+
 - LSIF uploads now respect the `-insecure-skip-verify` flag to insecurely (surprise!) skip TLS certificate validation when communicating with Sourcegraph. [#559](https://github.com/sourcegraph/src-cli/pull/559)
 
 ### Changed

internal/batches/batch_spec.go

@@ -47,7 +47,7 @@ type ChangesetTemplate struct {
 	Body      string                       `json:"body,omitempty" yaml:"body"`
 	Branch    string                       `json:"branch,omitempty" yaml:"branch"`
 	Commit    ExpandedGitCommitDescription `json:"commit,omitempty" yaml:"commit"`
-	Published overridable.BoolOrString     `json:"published" yaml:"published"`
+	Published *overridable.BoolOrString    `json:"published" yaml:"published"`
 }
 
 type GitCommitAuthor struct {

internal/batches/executor/changeset_specs.go

@@ -9,7 +9,9 @@ import (
 	"github.com/sourcegraph/src-cli/internal/batches"
 )
 
-func createChangesetSpecs(task *Task, result executionResult, autoAuthorDetails bool) ([]*batches.ChangesetSpec, error) {
+var errOptionalPublishedUnsupported = errors.New(`This Sourcegraph version requires the "published" field to be specified in the batch spec; upgrade to version 3.30.0 or later to be able to omit the published field and control publication from the UI.`)
+
+func createChangesetSpecs(task *Task, result executionResult, features batches.FeatureFlags) ([]*batches.ChangesetSpec, error) {
 	repo := task.Repository.Name
 
 	tmplCtx := &ChangesetTemplateContext{
@@ -26,7 +28,7 @@ func createChangesetSpecs(task *Task, result executionResult, autoAuthorDetails
 	var authorEmail string
 
 	if task.Template.Commit.Author == nil {
-		if autoAuthorDetails {
+		if features.IncludeAutoAuthorDetails {
 			// user did not provide author info, so use defaults
 			authorName = "Sourcegraph"
 			authorEmail = "[email protected]"
@@ -66,7 +68,14 @@ func createChangesetSpecs(task *Task, result executionResult, autoAuthorDetails
 		return nil, err
 	}
 
-	newSpec := func(branch, diff string) *batches.ChangesetSpec {
+	newSpec := func(branch, diff string) (*batches.ChangesetSpec, error) {
+		var published interface{} = nil
+		if task.Template.Published != nil {
+			published = task.Template.Published.ValueWithSuffix(repo, branch)
+		} else if !features.AllowOptionalPublished {
+			return nil, errOptionalPublishedUnsupported
+		}
+
 		return &batches.ChangesetSpec{
 			BaseRepository: task.Repository.ID,
 			CreatedChangeset: &batches.CreatedChangeset{
@@ -84,9 +93,9 @@ func createChangesetSpecs(task *Task, result executionResult, autoAuthorDetails
 						Diff:        diff,
 					},
 				},
-				Published: task.Template.Published.ValueWithSuffix(repo, branch),
+				Published: published,
 			},
-		}
+		}, nil
 	}
 
 	var specs []*batches.ChangesetSpec
@@ -105,10 +114,18 @@ func createChangesetSpecs(task *Task, result executionResult, autoAuthorDetails
 		}
 
 		for branch, diff := range diffsByBranch {
-			specs = append(specs, newSpec(branch, diff))
+			spec, err := newSpec(branch, diff)
+			if err != nil {
+				return nil, err
+			}
+			specs = append(specs, spec)
 		}
 	} else {
-		specs = append(specs, newSpec(defaultBranch, result.Diff))
+		spec, err := newSpec(defaultBranch, result.Diff)
+		if err != nil {
+			return nil, err
+		}
+		specs = append(specs, spec)
 	}
 
 	return specs, nil

internal/batches/executor/changeset_specs_test.go

@@ -80,18 +80,23 @@ func TestCreateChangesetSpecs(t *testing.T) {
 		Outputs: map[string]interface{}{},
 	}
 
+	featuresWithoutOptionalPublished := featuresAllEnabled()
+	featuresWithoutOptionalPublished.AllowOptionalPublished = false
+
 	tests := []struct {
-		name   string
-		task   *Task
-		result executionResult
+		name     string
+		task     *Task
+		features batches.FeatureFlags
+		result   executionResult
 
 		want    []*batches.ChangesetSpec
 		wantErr string
 	}{
 		{
-			name:   "success",
-			task:   defaultTask,
-			result: defaultResult,
+			name:     "success",
+			task:     defaultTask,
+			features: featuresAllEnabled(),
+			result:   defaultResult,
 			want: []*batches.ChangesetSpec{
 				defaultChangesetSpec,
 			},
@@ -103,7 +108,8 @@ func TestCreateChangesetSpecs(t *testing.T) {
 				published := `[{"github.com/sourcegraph/*@my-branch": true}]`
 				task.Template.Published = parsePublishedFieldString(t, published)
 			}),
-			result: defaultResult,
+			features: featuresAllEnabled(),
+			result:   defaultResult,
 			want: []*batches.ChangesetSpec{
 				specWith(defaultChangesetSpec, func(s *batches.ChangesetSpec) {
 					s.Published = true
@@ -117,19 +123,44 @@ func TestCreateChangesetSpecs(t *testing.T) {
 				published := `[{"github.com/sourcegraph/*@another-branch-name": true}]`
 				task.Template.Published = parsePublishedFieldString(t, published)
 			}),
-			result: defaultResult,
+			features: featuresAllEnabled(),
+			result:   defaultResult,
 			want: []*batches.ChangesetSpec{
 				specWith(defaultChangesetSpec, func(s *batches.ChangesetSpec) {
 					s.Published = false
 				}),
 			},
 			wantErr: "",
 		},
+		{
+			name: "publish in UI on a supported version",
+			task: taskWith(defaultTask, func(task *Task) {
+				task.Template.Published = nil
+			}),
+			features: featuresAllEnabled(),
+			result:   defaultResult,
+			want: []*batches.ChangesetSpec{
+				specWith(defaultChangesetSpec, func(s *batches.ChangesetSpec) {
+					s.Published = nil
+				}),
+			},
+			wantErr: "",
+		},
+		{
+			name: "publish in UI on an unsupported version",
+			task: taskWith(defaultTask, func(task *Task) {
+				task.Template.Published = nil
+			}),
+			features: featuresWithoutOptionalPublished,
+			result:   defaultResult,
+			want:     nil,
+			wantErr:  errOptionalPublishedUnsupported.Error(),
+		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			have, err := createChangesetSpecs(tt.task, tt.result, true)
+			have, err := createChangesetSpecs(tt.task, tt.result, tt.features)
 			if err != nil {
 				if tt.wantErr != "" {
 					if err.Error() != tt.wantErr {
@@ -308,12 +339,12 @@ func TestValidateGroups(t *testing.T) {
 	}
 }
 
-func parsePublishedFieldString(t *testing.T, input string) overridable.BoolOrString {
+func parsePublishedFieldString(t *testing.T, input string) *overridable.BoolOrString {
 	t.Helper()
 
 	var result overridable.BoolOrString
 	if err := json.Unmarshal([]byte(input), &result); err != nil {
 		t.Fatalf("failed to parse %q as overridable.BoolOrString: %s", input, err)
 	}
-	return result
+	return &result
 }

internal/batches/executor/coordinator.go

@@ -48,7 +48,7 @@ type NewCoordinatorOpts struct {
 	SkipErrors bool
 
 	// Used by createChangesetSpecs
-	AutoAuthorDetails bool
+	Features batches.FeatureFlags
 
 	CleanArchives bool
 	Parallelism   int
@@ -66,7 +66,7 @@ func NewCoordinator(opts NewCoordinatorOpts) *Coordinator {
 		Creator: opts.Creator,
 		Logger:  logManager,
 
-		AutoAuthorDetails: opts.AutoAuthorDetails,
+		AutoAuthorDetails: opts.Features.IncludeAutoAuthorDetails,
 		Parallelism:       opts.Parallelism,
 		Timeout:           opts.Timeout,
 		TempDir:           opts.TempDir,
@@ -131,7 +131,7 @@ func (c *Coordinator) checkCacheForTask(ctx context.Context, task *Task) (specs
 		return specs, true, nil
 	}
 
-	specs, err = createChangesetSpecs(task, result, c.opts.AutoAuthorDetails)
+	specs, err = createChangesetSpecs(task, result, c.opts.Features)
 	if err != nil {
 		return specs, false, err
 	}
@@ -199,7 +199,7 @@ func (c *Coordinator) cacheAndBuildSpec(ctx context.Context, taskResult taskResu
 	}
 
 	// Build the changeset specs.
-	specs, err = createChangesetSpecs(taskResult.task, taskResult.result, c.opts.AutoAuthorDetails)
+	specs, err = createChangesetSpecs(taskResult.task, taskResult.result, c.opts.Features)
 	if err != nil {
 		return specs, err
 	}

internal/batches/executor/coordinator_test.go

@@ -9,13 +9,15 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
+	"github.com/sourcegraph/batch-change-utils/overridable"
 	"github.com/sourcegraph/src-cli/internal/batches"
 	"github.com/sourcegraph/src-cli/internal/batches/git"
 	"github.com/sourcegraph/src-cli/internal/batches/graphql"
 	"github.com/sourcegraph/src-cli/internal/batches/mock"
 )
 
 func TestCoordinator_Execute(t *testing.T) {
+	publishedFalse := overridable.FromBoolOrString(false)
 	srcCLITask := &Task{Repository: testRepo1}
 	sourcegraphTask := &Task{Repository: testRepo2}
 
@@ -75,6 +77,7 @@ func TestCoordinator_Execute(t *testing.T) {
 					{task: sourcegraphTask, result: executionResult{Diff: `dummydiff2`}},
 				},
 			},
+			opts: NewCoordinatorOpts{Features: featuresAllEnabled()},
 
 			wantCacheEntries: 2,
 			wantSpecs: []*batches.ChangesetSpec{
@@ -116,6 +119,7 @@ func TestCoordinator_Execute(t *testing.T) {
 							Email: "output1=${{ outputs.output1}}",
 						},
 					},
+					Published: &publishedFalse,
 				},
 			},
 
@@ -141,6 +145,7 @@ func TestCoordinator_Execute(t *testing.T) {
 					},
 				},
 			},
+			opts: NewCoordinatorOpts{Features: featuresAllEnabled()},
 
 			wantCacheEntries: 1,
 			wantSpecs: []*batches.ChangesetSpec{
@@ -191,6 +196,7 @@ func TestCoordinator_Execute(t *testing.T) {
 					{task: sourcegraphTask, result: executionResult{Diff: nestedChangesDiff}},
 				},
 			},
+			opts: NewCoordinatorOpts{Features: featuresAllEnabled()},
 
 			// We have 4 ChangesetSpecs, but we only want 2 cache entries,
 			// since we cache per Task, not per resulting changeset spec.
@@ -216,7 +222,7 @@ func TestCoordinator_Execute(t *testing.T) {
 		},
 		{
 			name: "skip errors",
-			opts: NewCoordinatorOpts{SkipErrors: true},
+			opts: NewCoordinatorOpts{Features: featuresAllEnabled(), SkipErrors: true},
 
 			tasks: []*Task{srcCLITask, sourcegraphTask},
 

internal/batches/executor/executor_test.go

@@ -463,5 +463,6 @@ func featuresAllEnabled() batches.FeatureFlags {
 		AllowTransformChanges:    true,
 		AllowWorkspaces:          true,
 		AllowConditionalExec:     true,
+		AllowOptionalPublished:   true,
 	}
 }

internal/batches/executor/main_test.go

@@ -25,6 +25,8 @@ var testRepo2 = &graphql.Repository{
 	},
 }
 
+var testPublished = overridable.FromBoolOrString(false)
+
 var testChangesetTemplate = &batches.ChangesetTemplate{
 	Title:  "commit title",
 	Body:   "commit body",
@@ -36,5 +38,5 @@ var testChangesetTemplate = &batches.ChangesetTemplate{
 			Email: "[email protected]",
 		},
 	},
-	Published: overridable.FromBoolOrString(false),
+	Published: &testPublished,
 }

internal/batches/features.go

@@ -15,6 +15,7 @@ type FeatureFlags struct {
 	AllowWorkspaces          bool
 	BatchChanges             bool
 	AllowConditionalExec     bool
+	AllowOptionalPublished   bool
 }
 
 func (ff *FeatureFlags) SetFromVersion(version string) error {
@@ -30,6 +31,7 @@ func (ff *FeatureFlags) SetFromVersion(version string) error {
 		{&ff.AllowWorkspaces, ">= 3.25.0", "2021-01-29"},
 		{&ff.BatchChanges, ">= 3.26.0", "2021-03-07"},
 		{&ff.AllowConditionalExec, ">= 3.28.0", "2021-05-05"},
+		{&ff.AllowOptionalPublished, ">= 3.30.0", "2021-06-21"},
 	} {
 		value, err := api.CheckSourcegraphVersion(version, feature.constraint, feature.minDate)
 		if err != nil {

internal/batches/service/service.go

@@ -191,7 +191,7 @@ func (svc *Service) BuildTasks(ctx context.Context, repos []*graphql.Repository,
 func (svc *Service) NewCoordinator(opts executor.NewCoordinatorOpts) *executor.Coordinator {
 	opts.ResolveRepoName = svc.resolveRepositoryName
 	opts.Client = svc.client
-	opts.AutoAuthorDetails = svc.features.IncludeAutoAuthorDetails
+	opts.Features = svc.features
 
 	return executor.NewCoordinator(opts)
 }