Skip to content

Remove old feature gates from pre-4.0 #822

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
eseliger merged 6 commits into from
Aug 25, 2022
Merged

Remove old feature gates from pre-4.0 #822

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
be84e23
Remove old feature gates from pre-4.0
eseliger Aug 16, 2022
efd1d7f
Pin go module version
eseliger Aug 22, 2022
904ae9c
Bump version again
eseliger Aug 24, 2022
c42a94c
Bump version
eseliger Aug 24, 2022
2d67a78
Update cmd/src/batch_common.go
eseliger Aug 25, 2022
e8a226d
Fixups
eseliger Aug 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 16 additions & 26 deletions cmd/src/batch_common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,37 +177,17 @@ func batchDefaultCacheDir() string {
if err != nil {
return ""
}

// Check if there's an old campaigns cache directory but not a new batch
// directory: if so, we should rename the old directory and carry on.
//
// TODO(campaigns-deprecation): we can remove this migration shim after June
// 2021.
old := path.Join(uc, "sourcegraph", "campaigns")
dir := path.Join(uc, "sourcegraph", "batch")
if _, err := os.Stat(dir); os.IsNotExist(err) {
if _, err := os.Stat(old); os.IsExist(err) {
// We'll just try to do this without checking for an error: if it
// fails, we'll carry on and let the normal cache directory handling
// logic take care of it.
os.Rename(old, dir)
}
}

return dir
}

// batchDefaultTempDirPrefix returns the prefix to be passed to ioutil.TempFile.
// If one of the environment variables SRC_BATCH_TMP_DIR or
// SRC_CAMPAIGNS_TMP_DIR is set, that is used as the prefix. Otherwise we use
// "/tmp".
// If the environment variable SRC_BATCH_TMP_DIR is set, that is used as the prefix.
// Otherwise we use "/tmp".
func batchDefaultTempDirPrefix() string {
// TODO(campaigns-deprecation): we can remove this migration shim in
// Sourcegraph 4.0.
for _, env := range []string{"SRC_BATCH_TMP_DIR", "SRC_CAMPAIGNS_TMP_DIR"} {
if p := os.Getenv(env); p != "" {
return p
}
if p := os.Getenv("SRC_BATCH_TMP_DIR"); p != "" {
return p
}

// On macOS, we use an explicit prefix for our temp directories, because
Expand Down Expand Up @@ -280,7 +260,7 @@ func executeBatchSpec(ctx context.Context, ui ui.ExecUI, opts executeBatchSpecOp

imageCache := docker.NewImageCache()

if err := svc.DetermineFeatureFlags(ctx); err != nil {
if err := validateSourcegraphVersionConstraint(ctx, svc); err != nil {
return err
}

Expand Down Expand Up @@ -399,7 +379,6 @@ func executeBatchSpec(ctx context.Context, ui ui.ExecUI, opts executeBatchSpecOp
GlobalEnv: os.Environ(),
IsRemote: false,
},
Features: svc.Features(),
Logger: logManager,
Cache: executor.NewDiskCache(opts.flags.cacheDir),
GlobalEnv: os.Environ(),
Expand Down Expand Up @@ -604,3 +583,14 @@ func getBatchParallelism(ctx context.Context, flag int) (int, error) {

return docker.NCPU(ctx)
}

func validateSourcegraphVersionConstraint(ctx context.Context, svc *service.Service) error {
Copy link
Contributor

@LawnGnome LawnGnome Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worry a little that this may be too simplistic as we start adding things in 4.x again, but let's cross that bridge when we get to it.

ffs, err := svc.DetermineFeatureFlags(ctx)
if err != nil {
return err
}
if ffs.Sourcegraph40 {
return nil
}
return errors.Newf("\n\n * Warning:\n This version of src-cli requires Sourcegraph version 4.0 or newer. If you're not on Sourcegraph 4.0 or newer, please use the 3.x release of src-cli that corresponds to your Sourcegraph version.\n\n")
}
2 changes: 1 addition & 1 deletion cmd/src/batch_new.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ Examples:
Client: cfg.apiClient(apiFlags, flagSet.Output()),
})

if err := svc.DetermineFeatureFlags(ctx); err != nil {
if err := validateSourcegraphVersionConstraint(ctx, svc); err != nil {
return err
}

Expand Down
2 changes: 1 addition & 1 deletion cmd/src/batch_remote.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ Examples:
Client: cfg.apiClient(flags.api, flagSet.Output()),
})

if err := svc.DetermineFeatureFlags(ctx); err != nil {
if err := validateSourcegraphVersionConstraint(ctx, svc); err != nil {
return err
}

Expand Down
2 changes: 1 addition & 1 deletion cmd/src/batch_repositories.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ Examples:
Client: client,
})

if err := svc.DetermineFeatureFlags(ctx); err != nil {
if err := validateSourcegraphVersionConstraint(ctx, svc); err != nil {
return err
}

Expand Down
2 changes: 1 addition & 1 deletion cmd/src/batch_validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ Examples:
Client: cfg.apiClient(apiFlags, flagSet.Output()),
})

if err := svc.DetermineFeatureFlags(ctx); err != nil {
if err := validateSourcegraphVersionConstraint(ctx, svc); err != nil {
ui.ExecutionError(err)
return err
}
Expand Down
4 changes: 2 additions & 2 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ require (
github.com/sourcegraph/go-diff v0.6.1
github.com/sourcegraph/jsonx v0.0.0-20200629203448-1a936bd500cf
github.com/sourcegraph/scip v0.2.0
github.com/sourcegraph/sourcegraph/lib v0.0.0-20220822143138-d621aac70d3d
github.com/sourcegraph/sourcegraph/lib v0.0.0-20220825181731-397a768a5290
github.com/stretchr/testify v1.7.2
golang.org/x/net v0.0.0-20220722155237-a158d28d115b
golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4
Expand Down Expand Up @@ -103,7 +103,7 @@ require (
go.uber.org/zap v1.21.0 // indirect
golang.org/x/crypto v0.0.0-20210921155107-089bfa567519 // indirect
golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4 // indirect
golang.org/x/sys v0.0.0-20220818161305-2296e01440c6 // indirect
golang.org/x/sys v0.0.0-20220823224334-20c2bfdbfe24 // indirect
golang.org/x/term v0.0.0-20220411215600-e5f449aeb171 // indirect
golang.org/x/text v0.3.7 // indirect
golang.org/x/tools v0.1.12 // indirect
Expand Down
8 changes: 4 additions & 4 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,8 +370,8 @@ github.com/sourcegraph/log v0.0.0-20220707160925-6a936691c838 h1:8wknDSCUVYbaRT6
github.com/sourcegraph/log v0.0.0-20220707160925-6a936691c838/go.mod h1:zWEPlKrWBUVpko/tOgDS+qrp7BmzaCcmUrh9+ver1iQ=
github.com/sourcegraph/scip v0.2.0 h1:Z9rR9TNONtRhqcpm0JP/yEBUy0fBKaSVbWIZKih5v04=
github.com/sourcegraph/scip v0.2.0/go.mod h1:EYyT39nXdZDNVmgbJAlyIVWbEb1txnAOKpJPSYpvgXk=
github.com/sourcegraph/sourcegraph/lib v0.0.0-20220822143138-d621aac70d3d h1:bUaXAbtXSWbcv8LjDwwYXo0grERhlOJDn2T8ROe/Mmc=
github.com/sourcegraph/sourcegraph/lib v0.0.0-20220822143138-d621aac70d3d/go.mod h1:9wnFUNfpORLAOJn4XAO7ZeWnYkf6/CxlWaTU1vlpuKc=
github.com/sourcegraph/sourcegraph/lib v0.0.0-20220825181731-397a768a5290 h1:SLCu3Rf1eLZ4sNKl0Bg1oURTgDxEutRCaTQt5dpVqH4=
github.com/sourcegraph/sourcegraph/lib v0.0.0-20220825181731-397a768a5290/go.mod h1:9wnFUNfpORLAOJn4XAO7ZeWnYkf6/CxlWaTU1vlpuKc=
github.com/sourcegraph/yaml v1.0.1-0.20200714132230-56936252f152 h1:z/MpntplPaW6QW95pzcAR/72Z5TWDyDnSo0EOcyij9o=
github.com/sourcegraph/yaml v1.0.1-0.20200714132230-56936252f152/go.mod h1:GIjDIg/heH5DOkXY3YJ/wNhfHsQHoXGjl8G8amsYQ1I=
github.com/spf13/afero v1.1.2/go.mod h1:j4pytiNVoe2o6bmDsKpLACNPDBIoEAkihy7loJ1B0CQ=
Expand Down Expand Up @@ -535,8 +535,8 @@ golang.org/x/sys v0.0.0-20211102192858-4dd72447c267/go.mod h1:oPkhp1MJrh7nUepCBc
golang.org/x/sys v0.0.0-20211117180635-dee7805ff2e1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20211213223007-03aa0b5f6827/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20220209214540-3681064d5158/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20220818161305-2296e01440c6 h1:Sx/u41w+OwrInGdEckYmEuU5gHoGSL4QbDz3S9s6j4U=
golang.org/x/sys v0.0.0-20220818161305-2296e01440c6/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20220823224334-20c2bfdbfe24 h1:TyKJRhyo17yWxOMCTHKWrc5rddHORMlnZ/j57umaUd8=
golang.org/x/sys v0.0.0-20220823224334-20c2bfdbfe24/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
golang.org/x/term v0.0.0-20220411215600-e5f449aeb171 h1:EH1Deb8WZJ0xc0WK//leUHXcX9aLE5SymusoTmMZye8=
golang.org/x/term v0.0.0-20220411215600-e5f449aeb171/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8=
Expand Down
31 changes: 4 additions & 27 deletions internal/api/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
ioaux "github.com/jig/teereadcloser"
"github.com/kballard/go-shellquote"
"github.com/mattn/go-isatty"

"github.com/sourcegraph/src-cli/internal/version"
)

Expand All @@ -28,13 +29,6 @@ type Client interface {
// NewRequest creates a GraphQL request.
NewRequest(query string, vars map[string]interface{}) Request

// NewGzippedRequest creates a GraphQL request with gzip compression turned on.
NewGzippedRequest(query string, vars map[string]interface{}) Request

// NewGzippedQuery is a convenience wrapper around NewQuery with gzip
// compression turned on.
NewGzippedQuery(query string) Request

// NewHTTPRequest creates an http.Request for the Sourcegraph API.
//
// path is joined against the API route. For example on Sourcegraph.com this
Expand Down Expand Up @@ -72,7 +66,6 @@ type request struct {
client *client
query string
vars map[string]interface{}
gzip bool
}

// ClientOpts encapsulates the options given to NewClient.
Expand Down Expand Up @@ -132,19 +125,6 @@ func (c *client) NewRequest(query string, vars map[string]interface{}) Request {
}
}

func (c *client) NewGzippedRequest(query string, vars map[string]interface{}) Request {
return &request{
client: c,
query: query,
vars: vars,
gzip: true,
}
}

func (c *client) NewGzippedQuery(query string) Request {
return c.NewGzippedRequest(query, nil)
}

func (c *client) Do(req *http.Request) (*http.Response, error) {
return c.httpClient.Do(req)
}
Expand Down Expand Up @@ -216,19 +196,16 @@ func (r *request) do(ctx context.Context, result interface{}) (bool, error) {
}

var bufBody io.Reader = bytes.NewBuffer(reqBody)
if r.gzip {
bufBody = gzipReader(bufBody)
}
bufBody = gzipReader(bufBody)

// Create the HTTP request.
req, err := r.client.NewHTTPRequest(ctx, "POST", ".api/graphql", bufBody)
if err != nil {
return false, err
}

if r.gzip {
req.Header.Set("Content-Encoding", "gzip")
}
// Use gzip compression.
req.Header.Set("Content-Encoding", "gzip")

// Perform the request.
resp, err := r.client.httpClient.Do(req)
Expand Down
8 changes: 1 addition & 7 deletions internal/batches/executor/coordinator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"github.com/sourcegraph/sourcegraph/lib/batches/execution"
"github.com/sourcegraph/sourcegraph/lib/batches/execution/cache"

"github.com/sourcegraph/src-cli/internal/batches"
"github.com/sourcegraph/src-cli/internal/batches/log"
)

Expand All @@ -34,8 +33,6 @@ type NewCoordinatorOpts struct {
Logger log.LogManager
GlobalEnv []string

// Used by batcheslib.BuildChangesetSpecs
Features batches.FeatureFlags
IsRemote bool
}

Expand Down Expand Up @@ -124,10 +121,7 @@ func (c Coordinator) buildChangesetSpecs(task *Task, batchSpec *batcheslib.Batch
},
}

return batcheslib.BuildChangesetSpecs(input, batcheslib.ChangesetSpecFeatureFlags{
IncludeAutoAuthorDetails: c.opts.Features.IncludeAutoAuthorDetails,
AllowOptionalPublished: c.opts.Features.AllowOptionalPublished,
})
return batcheslib.BuildChangesetSpecs(input)
}

func (c *Coordinator) loadCachedStepResults(ctx context.Context, task *Task, globalEnv []string) error {
Expand Down
8 changes: 4 additions & 4 deletions internal/batches/executor/coordinator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func TestCoordinator_Execute(t *testing.T) {
{task: sourcegraphTask, stepResults: []execution.AfterStepResult{{Diff: `dummydiff2`}}},
},
},
opts: NewCoordinatorOpts{Features: featuresAllEnabled()},
opts: NewCoordinatorOpts{},

wantCacheEntries: 2,
wantSpecs: []*batcheslib.ChangesetSpec{
Expand Down Expand Up @@ -150,7 +150,7 @@ func TestCoordinator_Execute(t *testing.T) {
},
},
},
opts: NewCoordinatorOpts{Features: featuresAllEnabled()},
opts: NewCoordinatorOpts{},

wantCacheEntries: 1,
wantSpecs: []*batcheslib.ChangesetSpec{
Expand Down Expand Up @@ -201,7 +201,7 @@ func TestCoordinator_Execute(t *testing.T) {
{task: sourcegraphTask, stepResults: []execution.AfterStepResult{{Diff: nestedChangesDiff}}},
},
},
opts: NewCoordinatorOpts{Features: featuresAllEnabled()},
opts: NewCoordinatorOpts{},

// TODO: Fix comment.
// We have 4 ChangesetSpecs, but we only want 2 cache entries,
Expand Down Expand Up @@ -249,7 +249,7 @@ func TestCoordinator_Execute(t *testing.T) {
{task: sourcegraphTask, stepResults: []execution.AfterStepResult{{Diff: `dummydiff2`, StepIndex: 0}}},
},
},
opts: NewCoordinatorOpts{Features: featuresAllEnabled()},
opts: NewCoordinatorOpts{},

wantCacheEntries: 2,
wantSpecs: []*batcheslib.ChangesetSpec{
Expand Down
13 changes: 0 additions & 13 deletions internal/batches/executor/executor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (
"github.com/sourcegraph/sourcegraph/lib/batches/template"

"github.com/sourcegraph/src-cli/internal/api"
"github.com/sourcegraph/src-cli/internal/batches"
"github.com/sourcegraph/src-cli/internal/batches/docker"
"github.com/sourcegraph/src-cli/internal/batches/mock"
"github.com/sourcegraph/src-cli/internal/batches/repozip"
Expand Down Expand Up @@ -510,18 +509,6 @@ func addToPath(t *testing.T, relPath string) {
os.Setenv("PATH", fmt.Sprintf("%s%c%s", dummyDockerPath, os.PathListSeparator, os.Getenv("PATH")))
}

func featuresAllEnabled() batches.FeatureFlags {
return batches.FeatureFlags{
AllowArrayEnvironments: true,
IncludeAutoAuthorDetails: true,
UseGzipCompression: true,
AllowTransformChanges: true,
AllowWorkspaces: true,
AllowConditionalExec: true,
AllowOptionalPublished: true,
}
}

func TestExecutor_CachedStepResults(t *testing.T) {
t.Run("single step cached", func(t *testing.T) {
archive := mock.RepoArchive{
Expand Down
24 changes: 4 additions & 20 deletions internal/batches/features.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,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
AllowTransformChanges bool
AllowWorkspaces bool
BatchChanges bool
AllowConditionalExec bool
AllowOptionalPublished bool
ServerSideBatchChanges bool
BitbucketCloud bool
Sourcegraph40 bool
}

func (ff *FeatureFlags) SetFromVersion(version string) error {
Expand All @@ -34,16 +25,9 @@ func (ff *FeatureFlags) SetFromVersion(version string) error {
// ">= 3.23.0". However, the same version IS considered to satisfy the constraint
// "3.23.0-0". See
// https://github.com/Masterminds/semver#working-with-prerelease-versions for more.
{&ff.AllowArrayEnvironments, ">= 3.23.0-0", "2020-11-24"},
{&ff.IncludeAutoAuthorDetails, ">= 3.20.0-0", "2020-09-10"},
{&ff.UseGzipCompression, ">= 3.21.0-0", "2020-10-12"},
{&ff.AllowTransformChanges, ">= 3.23.0-0", "2020-12-11"},
{&ff.AllowWorkspaces, ">= 3.25.0-0", "2021-01-29"},
{&ff.BatchChanges, ">= 3.26.0-0", "2021-03-07"},
{&ff.AllowConditionalExec, ">= 3.28.0-0", "2021-05-05"},
{&ff.AllowOptionalPublished, ">= 3.30.0-0", "2021-06-21"},
{&ff.ServerSideBatchChanges, ">= 3.37.0-0", "2022-02-08"},
{&ff.BitbucketCloud, ">= 3.40.0-0", "2022-04-20"},
// Example usage:
// {&ff.FlagName, ">= 3.23.0-0", "2020-11-24"},
{&ff.Sourcegraph40, ">= 4.0.0-0", "2022-08-24"},
} {
value, err := api.CheckSourcegraphVersion(version, feature.constraint, feature.minDate)
if err != nil {
Expand Down
5 changes: 0 additions & 5 deletions internal/batches/graphql/batch_change.go
View file
Open in desktop

This file was deleted.

Loading