diff --git a/agent/agent_worker.go b/agent/agent_worker.go index b4727f9eed..c8fdb4fa47 100644 --- a/agent/agent_worker.go +++ b/agent/agent_worker.go @@ -651,7 +651,7 @@ func (a *AgentWorker) RunJob(ctx context.Context, acceptResponse *api.Job) error }) // Now that we've got a job to do, we can start it. - jr, err := NewJobRunner(a.logger, a.apiClient, JobRunnerConfig{ + jr, err := NewJobRunner(ctx, a.logger, a.apiClient, JobRunnerConfig{ Job: acceptResponse, JWKS: a.agentConfiguration.JobVerificationJWKS, Debug: a.debug, diff --git a/agent/artifact_uploader.go b/agent/artifact_uploader.go index 060c19e113..ee5fa515dd 100644 --- a/agent/artifact_uploader.go +++ b/agent/artifact_uploader.go @@ -15,7 +15,7 @@ import ( "time" "github.com/buildkite/agent/v3/api" - "github.com/buildkite/agent/v3/experiments" + "github.com/buildkite/agent/v3/internal/experiments" "github.com/buildkite/agent/v3/internal/mime" "github.com/buildkite/agent/v3/logger" "github.com/buildkite/agent/v3/pool" @@ -73,7 +73,7 @@ func NewArtifactUploader(l logger.Logger, ac APIClient, c ArtifactUploaderConfig func (a *ArtifactUploader) Upload(ctx context.Context) error { // Create artifact structs for all the files we need to upload - artifacts, err := a.Collect() + artifacts, err := a.Collect(ctx) if err != nil { return fmt.Errorf("collecting artifacts: %w", err) } @@ -107,7 +107,7 @@ func isDir(path string) bool { return fi.IsDir() } -func (a *ArtifactUploader) Collect() (artifacts []*api.Artifact, err error) { +func (a *ArtifactUploader) Collect(ctx context.Context) (artifacts []*api.Artifact, err error) { wd, err := os.Getwd() if err != nil { return nil, fmt.Errorf("getting working directory: %w", err) @@ -182,7 +182,7 @@ func (a *ArtifactUploader) Collect() (artifacts []*api.Artifact, err error) { return nil, fmt.Errorf("resolving relative path for file %s: %w", file, err) } - if experiments.IsEnabled(experiments.NormalisedUploadPaths) { + if experiments.IsEnabled(ctx, experiments.NormalisedUploadPaths) { // Convert any Windows paths to Unix/URI form path = filepath.ToSlash(path) } diff --git a/agent/artifact_uploader_test.go b/agent/artifact_uploader_test.go index e64c18d628..d88c4b3705 100644 --- a/agent/artifact_uploader_test.go +++ b/agent/artifact_uploader_test.go @@ -1,6 +1,7 @@ package agent import ( + "context" "fmt" "os" "path/filepath" @@ -8,7 +9,7 @@ import ( "testing" "github.com/buildkite/agent/v3/api" - "github.com/buildkite/agent/v3/experiments" + "github.com/buildkite/agent/v3/internal/experiments" "github.com/buildkite/agent/v3/logger" "github.com/stretchr/testify/assert" ) @@ -24,7 +25,7 @@ func findArtifact(artifacts []*api.Artifact, search string) *api.Artifact { } func TestCollect(t *testing.T) { - // t.Parallel() cannot be used with experiments.Enable + ctx := context.Background() wd, _ := os.Getwd() root := filepath.Join(wd, "..") @@ -100,24 +101,16 @@ func TestCollect(t *testing.T) { // path.Join function instead (which uses Unix/URI-style path separators, // regardless of platform) - experimentKey := experiments.NormalisedUploadPaths - experimentPrev := experiments.IsEnabled(experimentKey) - defer func() { - if experimentPrev { - experiments.Enable(experimentKey) - } else { - experiments.Disable(experimentKey) - } - }() - experiments.Disable(experimentKey) - artifactsWithoutExperimentEnabled, err := uploader.Collect() + ctxExpEnabled, _ := experiments.Enable(ctx, experiments.NormalisedUploadPaths) + ctxExpDisabled := experiments.Disable(ctx, experiments.NormalisedUploadPaths) + + artifactsWithoutExperimentEnabled, err := uploader.Collect(ctxExpDisabled) if err != nil { t.Fatalf("[normalised-upload-paths disabled] uploader.Collect() error = %v", err) } assert.Equal(t, 5, len(artifactsWithoutExperimentEnabled)) - experiments.Enable(experimentKey) - artifactsWithExperimentEnabled, err := uploader.Collect() + artifactsWithExperimentEnabled, err := uploader.Collect(ctxExpEnabled) if err != nil { t.Fatalf("[normalised-upload-paths enabled] uploader.Collect() error = %v", err) } @@ -167,6 +160,8 @@ func TestCollect(t *testing.T) { } func TestCollectThatDoesntMatchAnyFiles(t *testing.T) { + ctx := context.Background() + wd, _ := os.Getwd() root := filepath.Join(wd, "..") os.Chdir(root) @@ -181,7 +176,7 @@ func TestCollectThatDoesntMatchAnyFiles(t *testing.T) { }, ";"), }) - artifacts, err := uploader.Collect() + artifacts, err := uploader.Collect(ctx) if err != nil { t.Fatalf("uploader.Collect() error = %v", err) } @@ -190,6 +185,8 @@ func TestCollectThatDoesntMatchAnyFiles(t *testing.T) { } func TestCollectWithSomeGlobsThatDontMatchAnything(t *testing.T) { + ctx := context.Background() + wd, _ := os.Getwd() root := filepath.Join(wd, "..") os.Chdir(root) @@ -203,7 +200,7 @@ func TestCollectWithSomeGlobsThatDontMatchAnything(t *testing.T) { }, ";"), }) - artifacts, err := uploader.Collect() + artifacts, err := uploader.Collect(ctx) if err != nil { t.Fatalf("uploader.Collect() error = %v", err) } @@ -214,6 +211,8 @@ func TestCollectWithSomeGlobsThatDontMatchAnything(t *testing.T) { } func TestCollectWithSomeGlobsThatDontMatchAnythingFollowingSymlinks(t *testing.T) { + ctx := context.Background() + wd, _ := os.Getwd() root := filepath.Join(wd, "..") os.Chdir(root) @@ -229,7 +228,7 @@ func TestCollectWithSomeGlobsThatDontMatchAnythingFollowingSymlinks(t *testing.T GlobResolveFollowSymlinks: true, }) - artifacts, err := uploader.Collect() + artifacts, err := uploader.Collect(ctx) if err != nil { t.Fatalf("uploader.Collect() error = %v", err) } @@ -240,6 +239,8 @@ func TestCollectWithSomeGlobsThatDontMatchAnythingFollowingSymlinks(t *testing.T } func TestCollectWithDuplicateMatches(t *testing.T) { + ctx := context.Background() + wd, _ := os.Getwd() root := filepath.Join(wd, "..") os.Chdir(root) @@ -252,7 +253,7 @@ func TestCollectWithDuplicateMatches(t *testing.T) { }, ";"), }) - artifacts, err := uploader.Collect() + artifacts, err := uploader.Collect(ctx) if err != nil { t.Fatalf("uploader.Collect() error = %v", err) } @@ -274,6 +275,8 @@ func TestCollectWithDuplicateMatches(t *testing.T) { } func TestCollectWithDuplicateMatchesFollowingSymlinks(t *testing.T) { + ctx := context.Background() + wd, _ := os.Getwd() root := filepath.Join(wd, "..") os.Chdir(root) @@ -287,7 +290,7 @@ func TestCollectWithDuplicateMatchesFollowingSymlinks(t *testing.T) { GlobResolveFollowSymlinks: true, }) - artifacts, err := uploader.Collect() + artifacts, err := uploader.Collect(ctx) if err != nil { t.Fatalf("uploader.Collect() error = %v", err) } @@ -310,6 +313,8 @@ func TestCollectWithDuplicateMatchesFollowingSymlinks(t *testing.T) { } func TestCollectMatchesUploadSymlinks(t *testing.T) { + ctx := context.Background() + wd, _ := os.Getwd() root := filepath.Join(wd, "..") os.Chdir(root) @@ -322,7 +327,7 @@ func TestCollectMatchesUploadSymlinks(t *testing.T) { UploadSkipSymlinks: true, }) - artifacts, err := uploader.Collect() + artifacts, err := uploader.Collect(ctx) if err != nil { t.Fatalf("uploader.Collect() error = %v", err) } diff --git a/agent/integration/job_runner_integration_test.go b/agent/integration/job_runner_integration_test.go index e5629b6a1a..f241b632a4 100644 --- a/agent/integration/job_runner_integration_test.go +++ b/agent/integration/job_runner_integration_test.go @@ -1,6 +1,7 @@ package integration import ( + "context" "fmt" "os" "strconv" @@ -13,6 +14,7 @@ import ( func TestPreBootstrapHookRefusesJob(t *testing.T) { t.Parallel() + ctx := context.Background() hooksDir, err := os.MkdirTemp("", "bootstrap-hooks") if err != nil { @@ -45,7 +47,7 @@ func TestPreBootstrapHookRefusesJob(t *testing.T) { mb.Expect().NotCalled() // The bootstrap won't be called, as the pre-bootstrap hook failed defer mb.CheckAndClose(t) - runJob(t, testRunJobConfig{ + runJob(t, ctx, testRunJobConfig{ job: j, server: server, agentCfg: agent.AgentConfiguration{HooksPath: hooksDir}, @@ -65,6 +67,7 @@ func TestPreBootstrapHookRefusesJob(t *testing.T) { func TestJobRunner_WhenBootstrapExits_ItSendsTheExitStatusToTheAPI(t *testing.T) { t.Parallel() + ctx := context.Background() exits := []int{0, 1, 2, 3} for _, exit := range exits { @@ -89,7 +92,7 @@ func TestJobRunner_WhenBootstrapExits_ItSendsTheExitStatusToTheAPI(t *testing.T) server := e.server("my-job-id") defer server.Close() - runJob(t, testRunJobConfig{ + runJob(t, ctx, testRunJobConfig{ job: j, server: server, agentCfg: agent.AgentConfiguration{}, @@ -106,6 +109,7 @@ func TestJobRunner_WhenBootstrapExits_ItSendsTheExitStatusToTheAPI(t *testing.T) func TestJobRunner_WhenJobHasToken_ItOverridesAccessToken(t *testing.T) { t.Parallel() + ctx := context.Background() jobToken := "actually-llamas-are-only-okay" @@ -133,7 +137,7 @@ func TestJobRunner_WhenJobHasToken_ItOverridesAccessToken(t *testing.T) { server := e.server("my-job-id") defer server.Close() - runJob(t, testRunJobConfig{ + runJob(t, ctx, testRunJobConfig{ job: j, server: server, agentCfg: agent.AgentConfiguration{}, @@ -145,6 +149,7 @@ func TestJobRunner_WhenJobHasToken_ItOverridesAccessToken(t *testing.T) { // Maybe that the job runner pulls the access token from the API client? but that's all handled in the `runJob` helper... func TestJobRunnerPassesAccessTokenToBootstrap(t *testing.T) { t.Parallel() + ctx := context.Background() j := &api.Job{ ID: "my-job-id", @@ -169,7 +174,7 @@ func TestJobRunnerPassesAccessTokenToBootstrap(t *testing.T) { server := e.server("my-job-id") defer server.Close() - runJob(t, testRunJobConfig{ + runJob(t, ctx, testRunJobConfig{ job: j, server: server, agentCfg: agent.AgentConfiguration{}, @@ -179,6 +184,7 @@ func TestJobRunnerPassesAccessTokenToBootstrap(t *testing.T) { func TestJobRunnerIgnoresPipelineChangesToProtectedVars(t *testing.T) { t.Parallel() + ctx := context.Background() j := &api.Job{ ID: "my-job-id", @@ -204,7 +210,7 @@ func TestJobRunnerIgnoresPipelineChangesToProtectedVars(t *testing.T) { server := e.server("my-job-id") defer server.Close() - runJob(t, testRunJobConfig{ + runJob(t, ctx, testRunJobConfig{ job: j, server: server, agentCfg: agent.AgentConfiguration{CommandEval: true}, diff --git a/agent/integration/job_verification_integration_test.go b/agent/integration/job_verification_integration_test.go index 78f1de8081..d5b99c637e 100644 --- a/agent/integration/job_verification_integration_test.go +++ b/agent/integration/job_verification_integration_test.go @@ -1,6 +1,7 @@ package integration import ( + "context" "fmt" "strings" "testing" @@ -91,6 +92,7 @@ var ( func TestJobVerification(t *testing.T) { t.Parallel() + ctx := context.Background() cases := []struct { name string @@ -227,7 +229,7 @@ func TestJobVerification(t *testing.T) { defer mb.CheckAndClose(t) tc.job.Step = signStep(t, pipelineUploadEnv, tc.job.Step, tc.signingKey) - runJob(t, testRunJobConfig{ + runJob(t, ctx, testRunJobConfig{ job: &tc.job, server: server, agentCfg: tc.agentConf, diff --git a/agent/integration/test_helpers.go b/agent/integration/test_helpers.go index 0b81b89406..a73c7d0641 100644 --- a/agent/integration/test_helpers.go +++ b/agent/integration/test_helpers.go @@ -47,7 +47,7 @@ type testRunJobConfig struct { verificationJWKS jwk.Set } -func runJob(t *testing.T, cfg testRunJobConfig) { +func runJob(t *testing.T, ctx context.Context, cfg testRunJobConfig) { t.Helper() l := logger.Discard @@ -64,7 +64,7 @@ func runJob(t *testing.T, cfg testRunJobConfig) { Token: "llamasrock", }) - jr, err := agent.NewJobRunner(l, client, agent.JobRunnerConfig{ + jr, err := agent.NewJobRunner(ctx, l, client, agent.JobRunnerConfig{ Job: cfg.job, JWKS: cfg.verificationJWKS, AgentConfiguration: cfg.agentCfg, diff --git a/agent/job_runner.go b/agent/job_runner.go index 2731afcf5e..38ca3f9bf3 100644 --- a/agent/job_runner.go +++ b/agent/job_runner.go @@ -12,7 +12,7 @@ import ( "time" "github.com/buildkite/agent/v3/api" - "github.com/buildkite/agent/v3/experiments" + "github.com/buildkite/agent/v3/internal/experiments" "github.com/buildkite/agent/v3/internal/job/shell" "github.com/buildkite/agent/v3/kubernetes" "github.com/buildkite/agent/v3/logger" @@ -162,7 +162,7 @@ type jobAPI interface { var _ jobRunner = (*JobRunner)(nil) // Initializes the job runner -func NewJobRunner(l logger.Logger, apiClient APIClient, conf JobRunnerConfig) (jobRunner, error) { +func NewJobRunner(ctx context.Context, l logger.Logger, apiClient APIClient, conf JobRunnerConfig) (jobRunner, error) { r := &JobRunner{ logger: l, conf: conf, @@ -220,7 +220,7 @@ func NewJobRunner(l logger.Logger, apiClient APIClient, conf JobRunnerConfig) (j r.envFile = file } - env, err := r.createEnvironment() + env, err := r.createEnvironment(ctx) if err != nil { return nil, err } @@ -333,7 +333,7 @@ func NewJobRunner(l logger.Logger, apiClient APIClient, conf JobRunnerConfig) (j processEnv := append(os.Environ(), env...) // The process that will run the bootstrap script - if experiments.IsEnabled(experiments.KubernetesExec) { + if experiments.IsEnabled(ctx, experiments.KubernetesExec) { containerCount, err := strconv.Atoi(os.Getenv("BUILDKITE_CONTAINER_COUNT")) if err != nil { return nil, fmt.Errorf("failed to parse BUILDKITE_CONTAINER_COUNT: %w", err) @@ -392,7 +392,7 @@ func (r *JobRunner) normalizeVerificationBehavior(behavior string) (string, erro } // Creates the environment variables that will be used in the process and writes a flat environment file -func (r *JobRunner) createEnvironment() ([]string, error) { +func (r *JobRunner) createEnvironment(ctx context.Context) ([]string, error) { // Create a clone of our jobs environment. We'll then set the // environment variables provided by the agent, which will override any // sent by Buildkite. The variables below should always take @@ -476,7 +476,7 @@ func (r *JobRunner) createEnvironment() ([]string, error) { env["BUILDKITE_GIT_CLEAN_FLAGS"] = r.conf.AgentConfiguration.GitCleanFlags env["BUILDKITE_GIT_MIRRORS_LOCK_TIMEOUT"] = fmt.Sprintf("%d", r.conf.AgentConfiguration.GitMirrorsLockTimeout) env["BUILDKITE_SHELL"] = r.conf.AgentConfiguration.Shell - env["BUILDKITE_AGENT_EXPERIMENT"] = strings.Join(experiments.Enabled(), ",") + env["BUILDKITE_AGENT_EXPERIMENT"] = strings.Join(experiments.Enabled(ctx), ",") env["BUILDKITE_REDACTED_VARS"] = strings.Join(r.conf.AgentConfiguration.RedactedVars, ",") env["BUILDKITE_STRICT_SINGLE_HOOKS"] = fmt.Sprintf("%t", r.conf.AgentConfiguration.StrictSingleHooks) diff --git a/agent/tags.go b/agent/tags.go index c55ccf87a2..9ba29f3ce6 100644 --- a/agent/tags.go +++ b/agent/tags.go @@ -10,7 +10,7 @@ import ( "strings" "time" - "github.com/buildkite/agent/v3/experiments" + "github.com/buildkite/agent/v3/internal/experiments" "github.com/buildkite/agent/v3/logger" "github.com/buildkite/roko" "github.com/denisbrodbeck/machineid" @@ -78,7 +78,7 @@ type tagFetcher struct { func (t *tagFetcher) Fetch(ctx context.Context, l logger.Logger, conf FetchTagsConfig) []string { tags := conf.Tags - if experiments.IsEnabled(experiments.KubernetesExec) { + if experiments.IsEnabled(ctx, experiments.KubernetesExec) { k8sTags, err := t.k8s() if err != nil { l.Warn("Could not fetch tags from k8s: %s", err) diff --git a/clicommand/acknowledgements.go b/clicommand/acknowledgements.go index 1890adb925..f7d180fdd7 100644 --- a/clicommand/acknowledgements.go +++ b/clicommand/acknowledgements.go @@ -2,6 +2,7 @@ package clicommand import ( "compress/gzip" + "context" "embed" "fmt" "io" @@ -32,7 +33,8 @@ var AcknowledgementsCommand = cli.Command{ Usage: "Prints the licenses and notices of open source software incorporated into this software.", Description: acknowledgementsHelpDescription, Action: func(c *cli.Context) error { - _, _, _, done := setupLoggerAndConfig[AcknowledgementsConfig](c) + ctx := context.Background() + _, _, _, _, done := setupLoggerAndConfig[AcknowledgementsConfig](ctx, c) defer done() // The main acknowledgements file should be generated by diff --git a/clicommand/agent_start.go b/clicommand/agent_start.go index 90e90bf34d..97056fe1fc 100644 --- a/clicommand/agent_start.go +++ b/clicommand/agent_start.go @@ -18,9 +18,9 @@ import ( "github.com/buildkite/agent/v3/agent" "github.com/buildkite/agent/v3/api" - "github.com/buildkite/agent/v3/experiments" "github.com/buildkite/agent/v3/hook" "github.com/buildkite/agent/v3/internal/agentapi" + "github.com/buildkite/agent/v3/internal/experiments" "github.com/buildkite/agent/v3/internal/job/shell" "github.com/buildkite/agent/v3/internal/utils" "github.com/buildkite/agent/v3/logger" @@ -168,7 +168,7 @@ type AgentStartConfig struct { DisconnectAfterJobTimeout int `cli:"disconnect-after-job-timeout" deprecated:"Use disconnect-after-idle-timeout instead"` } -func (asc AgentStartConfig) Features() []string { +func (asc AgentStartConfig) Features(ctx context.Context) []string { if asc.NoFeatureReporting { return []string{} } @@ -207,7 +207,7 @@ func (asc AgentStartConfig) Features() []string { features = append(features, "no-script-eval") } - for _, exp := range experiments.Enabled() { + for _, exp := range experiments.Enabled(ctx) { features = append(features, fmt.Sprintf("experiment-%s", exp)) } @@ -679,7 +679,7 @@ var AgentStartCommand = cli.Command{ }, Action: func(c *cli.Context) { ctx := context.Background() - cfg, l, configFile, done := setupLoggerAndConfig[AgentStartConfig](c, withConfigFilePaths( + ctx, cfg, l, configFile, done := setupLoggerAndConfig[AgentStartConfig](ctx, c, withConfigFilePaths( defaultConfigFilePaths(), )) defer done() @@ -804,7 +804,7 @@ var AgentStartCommand = cli.Command{ l.Fatal("The given tracing backend %q is not supported. Valid backends are: %q", cfg.TracingBackend, maps.Keys(tracetools.ValidTracingBackends)) } - if experiments.IsEnabled(experiments.AgentAPI) { + if experiments.IsEnabled(ctx, experiments.AgentAPI) { shutdown := runAgentAPI(ctx, l, cfg.SocketsPath) defer shutdown() } @@ -979,7 +979,7 @@ var AgentStartCommand = cli.Command{ // dispatches if it's being booted to acquire a // specific job. IgnoreInDispatches: cfg.AcquireJob != "", - Features: cfg.Features(), + Features: cfg.Features(ctx), } // Spawning multiple agents doesn't work if the agent is being @@ -1002,7 +1002,7 @@ var AgentStartCommand = cli.Command{ if cfg.SpawnWithPriority { p := i - if experiments.IsEnabled(experiments.DescendingSpawnPrioity) { + if experiments.IsEnabled(ctx, experiments.DescendingSpawnPrioity) { // This experiment helps jobs be assigned across all hosts // in cases where the value of --spawn varies between hosts. p = -i diff --git a/clicommand/annotate.go b/clicommand/annotate.go index 99e652aa45..9f427697bb 100644 --- a/clicommand/annotate.go +++ b/clicommand/annotate.go @@ -116,7 +116,7 @@ var AnnotateCommand = cli.Command{ }, Action: func(c *cli.Context) { ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[AnnotateConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[AnnotateConfig](ctx, c) defer done() if err := annotate(ctx, cfg, l); err != nil { diff --git a/clicommand/annotation_remove.go b/clicommand/annotation_remove.go index a5daff744d..d7f9d2b103 100644 --- a/clicommand/annotation_remove.go +++ b/clicommand/annotation_remove.go @@ -76,7 +76,7 @@ var AnnotationRemoveCommand = cli.Command{ }, Action: func(c *cli.Context) error { ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[AnnotationRemoveConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[AnnotationRemoveConfig](ctx, c) defer done() // Create the API client diff --git a/clicommand/artifact_download.go b/clicommand/artifact_download.go index f8c82ff361..5f0c801927 100644 --- a/clicommand/artifact_download.go +++ b/clicommand/artifact_download.go @@ -103,7 +103,7 @@ var ArtifactDownloadCommand = cli.Command{ }, Action: func(c *cli.Context) { ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[ArtifactDownloadConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[ArtifactDownloadConfig](ctx, c) defer done() // Create the API client diff --git a/clicommand/artifact_search.go b/clicommand/artifact_search.go index 685074510f..41f237432b 100644 --- a/clicommand/artifact_search.go +++ b/clicommand/artifact_search.go @@ -135,7 +135,7 @@ var ArtifactSearchCommand = cli.Command{ }, Action: func(c *cli.Context) error { ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[ArtifactSearchConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[ArtifactSearchConfig](ctx, c) defer done() // Create the API client diff --git a/clicommand/artifact_shasum.go b/clicommand/artifact_shasum.go index 2bf26bc866..d075c1c89b 100644 --- a/clicommand/artifact_shasum.go +++ b/clicommand/artifact_shasum.go @@ -111,7 +111,7 @@ var ArtifactShasumCommand = cli.Command{ }, Action: func(c *cli.Context) { ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[ArtifactShasumConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[ArtifactShasumConfig](ctx, c) defer done() if err := searchAndPrintShaSum(ctx, cfg, l, os.Stdout); err != nil { diff --git a/clicommand/artifact_upload.go b/clicommand/artifact_upload.go index 53cb58fdd0..22852b0848 100644 --- a/clicommand/artifact_upload.go +++ b/clicommand/artifact_upload.go @@ -140,7 +140,7 @@ var ArtifactUploadCommand = cli.Command{ }, Action: func(c *cli.Context) { ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[ArtifactUploadConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[ArtifactUploadConfig](ctx, c) defer done() // Create the API client diff --git a/clicommand/bootstrap.go b/clicommand/bootstrap.go index 4f82758862..d6541d805f 100644 --- a/clicommand/bootstrap.go +++ b/clicommand/bootstrap.go @@ -363,7 +363,7 @@ var BootstrapCommand = cli.Command{ }, Action: func(c *cli.Context) { ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[BootstrapConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[BootstrapConfig](ctx, c) defer done() // Turn of PTY support if we're on Windows diff --git a/clicommand/env_dump.go b/clicommand/env_dump.go index 11ba5ed090..6399cfe12e 100644 --- a/clicommand/env_dump.go +++ b/clicommand/env_dump.go @@ -1,6 +1,7 @@ package clicommand import ( + "context" "encoding/json" "os" @@ -53,7 +54,7 @@ var EnvDumpCommand = cli.Command{ ProfileFlag, }, Action: func(c *cli.Context) error { - cfg, l, _, done := setupLoggerAndConfig[EnvDumpConfig](c) + _, cfg, l, _, done := setupLoggerAndConfig[EnvDumpConfig](context.Background(), c) defer done() envn := os.Environ() diff --git a/clicommand/env_get.go b/clicommand/env_get.go index eb2f2133a8..29b16367a6 100644 --- a/clicommand/env_get.go +++ b/clicommand/env_get.go @@ -97,7 +97,7 @@ var EnvGetCommand = cli.Command{ func envGetAction(c *cli.Context) error { ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[EnvGetConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[EnvGetConfig](ctx, c) defer done() client, err := jobapi.NewDefaultClient(ctx) diff --git a/clicommand/env_set.go b/clicommand/env_set.go index 3af790665d..00a803cc50 100644 --- a/clicommand/env_set.go +++ b/clicommand/env_set.go @@ -83,7 +83,7 @@ var EnvSetCommand = cli.Command{ func envSetAction(c *cli.Context) error { ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[EnvSetConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[EnvSetConfig](ctx, c) defer done() client, err := jobapi.NewDefaultClient(ctx) diff --git a/clicommand/env_unset.go b/clicommand/env_unset.go index 9fe125e444..46c8d50caa 100644 --- a/clicommand/env_unset.go +++ b/clicommand/env_unset.go @@ -81,7 +81,7 @@ var EnvUnsetCommand = cli.Command{ func envUnsetAction(c *cli.Context) error { ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[EnvUnsetConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[EnvUnsetConfig](ctx, c) defer done() client, err := jobapi.NewDefaultClient(ctx) diff --git a/clicommand/global.go b/clicommand/global.go index b66e2389b1..9b6bd1f836 100644 --- a/clicommand/global.go +++ b/clicommand/global.go @@ -1,6 +1,7 @@ package clicommand import ( + "context" "errors" "fmt" "os" @@ -9,7 +10,7 @@ import ( "github.com/buildkite/agent/v3/api" "github.com/buildkite/agent/v3/cliconfig" - "github.com/buildkite/agent/v3/experiments" + "github.com/buildkite/agent/v3/internal/experiments" "github.com/buildkite/agent/v3/logger" "github.com/buildkite/agent/v3/version" "github.com/oleiade/reflections" @@ -182,23 +183,28 @@ func HandleProfileFlag(l logger.Logger, cfg any) func() { return func() {} } -func HandleGlobalFlags(l logger.Logger, cfg any) func() { +func HandleGlobalFlags(ctx context.Context, l logger.Logger, cfg any) (context.Context, func()) { // Enable experiments experimentNames, err := reflections.GetField(cfg, "Experiments") - if err == nil { - experimentNamesSlice, ok := experimentNames.([]string) - if ok { - for _, name := range experimentNamesSlice { - state := experiments.EnableWithWarnings(l, name) - if state == experiments.StateKnown { - l.Debug("Enabled experiment %q", name) - } - } + if err != nil { + return ctx, HandleProfileFlag(l, cfg) + } + + experimentNamesSlice, ok := experimentNames.([]string) + if !ok { + return ctx, HandleProfileFlag(l, cfg) + } + + for _, name := range experimentNamesSlice { + nctx, state := experiments.EnableWithWarnings(ctx, l, name) + if state == experiments.StateKnown { + l.Debug("Enabled experiment %q", name) } + ctx = nctx } // Handle profiling flag - return HandleProfileFlag(l, cfg) + return ctx, HandleProfileFlag(l, cfg) } func handleLogLevelFlag(l logger.Logger, cfg any) error { @@ -287,7 +293,8 @@ func withConfigFilePaths(paths []string) func(*cliconfig.Loader) { // future to clean up other resources. Importantly, the calling code does not // need to know or care about what the returned function does, only that it // must defer it. -func setupLoggerAndConfig[T any](c *cli.Context, opts ...configOpts) ( +func setupLoggerAndConfig[T any](ctx context.Context, c *cli.Context, opts ...configOpts) ( + newCtx context.Context, cfg T, l logger.Logger, f *cliconfig.File, @@ -319,5 +326,6 @@ func setupLoggerAndConfig[T any](c *cli.Context, opts ...configOpts) ( } // Setup any global configuration options - return cfg, l, loader.File, HandleGlobalFlags(l, cfg) + ctx, done = HandleGlobalFlags(ctx, l, cfg) + return ctx, cfg, l, loader.File, done } diff --git a/clicommand/lock_acquire.go b/clicommand/lock_acquire.go index da22ae70f2..63162472a3 100644 --- a/clicommand/lock_acquire.go +++ b/clicommand/lock_acquire.go @@ -79,7 +79,7 @@ func lockAcquireAction(c *cli.Context) error { key := c.Args()[0] ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[LockAcquireConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[LockAcquireConfig](ctx, c) defer done() if cfg.LockScope != "machine" { diff --git a/clicommand/lock_do.go b/clicommand/lock_do.go index ca2e8d2e5f..3e3474bef4 100644 --- a/clicommand/lock_do.go +++ b/clicommand/lock_do.go @@ -84,7 +84,7 @@ func lockDoAction(c *cli.Context) error { key := c.Args()[0] ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[LockDoConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[LockDoConfig](ctx, c) defer done() if cfg.LockScope != "machine" { diff --git a/clicommand/lock_done.go b/clicommand/lock_done.go index 663866e2b0..2a36ef375e 100644 --- a/clicommand/lock_done.go +++ b/clicommand/lock_done.go @@ -58,7 +58,7 @@ func lockDoneAction(c *cli.Context) error { key := c.Args()[0] ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[LockDoneConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[LockDoneConfig](ctx, c) defer done() if cfg.LockScope != "machine" { diff --git a/clicommand/lock_get.go b/clicommand/lock_get.go index 76e25e90f9..4d298c063d 100644 --- a/clicommand/lock_get.go +++ b/clicommand/lock_get.go @@ -59,7 +59,7 @@ func lockGetAction(c *cli.Context) error { key := c.Args()[0] ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[LockGetConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[LockGetConfig](ctx, c) defer done() if cfg.LockScope != "machine" { diff --git a/clicommand/lock_release.go b/clicommand/lock_release.go index 2f25d7d1a4..2c1b16b82b 100644 --- a/clicommand/lock_release.go +++ b/clicommand/lock_release.go @@ -59,7 +59,7 @@ func lockReleaseAction(c *cli.Context) error { key, token := c.Args()[0], c.Args()[1] ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[LockReleaseConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[LockReleaseConfig](ctx, c) defer done() if cfg.LockScope != "machine" { diff --git a/clicommand/meta_data_exists.go b/clicommand/meta_data_exists.go index 19cd0b5fcb..483a4f8f33 100644 --- a/clicommand/meta_data_exists.go +++ b/clicommand/meta_data_exists.go @@ -75,7 +75,7 @@ var MetaDataExistsCommand = cli.Command{ }, Action: func(c *cli.Context) { ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[MetaDataExistsConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[MetaDataExistsConfig](ctx, c) defer done() // Create the API client diff --git a/clicommand/meta_data_get.go b/clicommand/meta_data_get.go index bd18337cbd..1a50cc6d9a 100644 --- a/clicommand/meta_data_get.go +++ b/clicommand/meta_data_get.go @@ -80,7 +80,7 @@ var MetaDataGetCommand = cli.Command{ }, Action: func(c *cli.Context) { ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[MetaDataGetConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[MetaDataGetConfig](ctx, c) defer done() // Create the API client diff --git a/clicommand/meta_data_keys.go b/clicommand/meta_data_keys.go index 3fa94021ec..23be3111b7 100644 --- a/clicommand/meta_data_keys.go +++ b/clicommand/meta_data_keys.go @@ -74,7 +74,7 @@ var MetaDataKeysCommand = cli.Command{ }, Action: func(c *cli.Context) { ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[MetaDataKeysConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[MetaDataKeysConfig](ctx, c) defer done() // Create the API client diff --git a/clicommand/meta_data_set.go b/clicommand/meta_data_set.go index 1928502af9..aa67fed310 100644 --- a/clicommand/meta_data_set.go +++ b/clicommand/meta_data_set.go @@ -78,7 +78,7 @@ var MetaDataSetCommand = cli.Command{ }, Action: func(c *cli.Context) { ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[MetaDataSetConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[MetaDataSetConfig](ctx, c) defer done() // Read the value from STDIN if argument omitted entirely diff --git a/clicommand/oidc_request_token.go b/clicommand/oidc_request_token.go index 2c6f66fbaa..ecaa0b5e91 100644 --- a/clicommand/oidc_request_token.go +++ b/clicommand/oidc_request_token.go @@ -94,7 +94,7 @@ var OIDCRequestTokenCommand = cli.Command{ }, Action: func(c *cli.Context) error { ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[OIDCTokenConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[OIDCTokenConfig](ctx, c) defer done() // Note: if --lifetime is omitted, cfg.Lifetime = 0 diff --git a/clicommand/pipeline_upload.go b/clicommand/pipeline_upload.go index bc6e0bc97c..05c775eb1e 100644 --- a/clicommand/pipeline_upload.go +++ b/clicommand/pipeline_upload.go @@ -161,7 +161,7 @@ var PipelineUploadCommand = cli.Command{ }, Action: func(c *cli.Context) { ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[PipelineUploadConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[PipelineUploadConfig](ctx, c) defer done() // Find the pipeline either from STDIN or the first argument diff --git a/clicommand/step_get.go b/clicommand/step_get.go index d321bf6048..1fc2db775f 100644 --- a/clicommand/step_get.go +++ b/clicommand/step_get.go @@ -89,7 +89,7 @@ var StepGetCommand = cli.Command{ }, Action: func(c *cli.Context) error { ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[StepGetConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[StepGetConfig](ctx, c) defer done() // Create the API client diff --git a/clicommand/step_update.go b/clicommand/step_update.go index f78596e3ab..960a34fc72 100644 --- a/clicommand/step_update.go +++ b/clicommand/step_update.go @@ -95,7 +95,7 @@ var StepUpdateCommand = cli.Command{ }, Action: func(c *cli.Context) { ctx := context.Background() - cfg, l, _, done := setupLoggerAndConfig[StepUpdateConfig](c) + ctx, cfg, l, _, done := setupLoggerAndConfig[StepUpdateConfig](ctx, c) defer done() // Read the value from STDIN if argument omitted entirely diff --git a/experiments/experiments.go b/internal/experiments/experiments.go similarity index 62% rename from experiments/experiments.go rename to internal/experiments/experiments.go index 2abc16bfcc..394288f58c 100644 --- a/experiments/experiments.go +++ b/internal/experiments/experiments.go @@ -5,7 +5,9 @@ package experiments import ( + "context" "fmt" + "sync" "github.com/buildkite/agent/v3/logger" ) @@ -58,20 +60,23 @@ var ( InbuiltStatusPage: standardPromotionMsg(InbuiltStatusPage, "v3.48.0"), } - experiments = make(map[string]bool, len(Available)) + // Used to track experiments possibly in use. + allMu sync.Mutex + all = make(map[string]struct{}) ) func standardPromotionMsg(key, version string) string { return fmt.Sprintf("The %s experiment has been promoted to a stable feature in agent version %s. You can safely remove the `--experiment %s` flag to silence this message and continue using the feature", key, version, key) } -func EnableWithUndo(key string) func() { - Enable(key) - return func() { Disable(key) } +type experimentCtxKey struct { + experiment string } -func EnableWithWarnings(l logger.Logger, key string) State { - state := Enable(key) +// EnableWithWarnings enables an experiment in a new context, logging +// information about unknown and promoted experiments. +func EnableWithWarnings(ctx context.Context, l logger.Logger, key string) (context.Context, State) { + newctx, state := Enable(ctx, key) switch state { case StateKnown: // Noop @@ -80,39 +85,52 @@ func EnableWithWarnings(l logger.Logger, key string) State { case StatePromoted: l.Warn(Promoted[key]) } - return state + return newctx, state } -// Enable a particular experiment in the agent. -func Enable(key string) (state State) { - experiments[key] = true +// Enable a particular experiment in a new context. +func Enable(ctx context.Context, key string) (newctx context.Context, state State) { + allMu.Lock() + all[key] = struct{}{} + allMu.Unlock() + + newctx = context.WithValue(ctx, experimentCtxKey{key}, true) if _, promoted := Promoted[key]; promoted { - return StatePromoted + return newctx, StatePromoted } if _, known := Available[key]; known { - return StateKnown + return newctx, StateKnown } - return StateUnknown + return newctx, StateUnknown } -// Disable a particular experiment in the agent. -func Disable(key string) { - delete(experiments, key) +// Disable a particular experiment in a new context. +func Disable(ctx context.Context, key string) context.Context { + // Even if we learn about the experiment through disablement, it is still + // an experiment... + allMu.Lock() + all[key] = struct{}{} + allMu.Unlock() + + return context.WithValue(ctx, experimentCtxKey{key}, false) } -// IsEnabled reports whether the named experiment is enabled. -func IsEnabled(key string) bool { - return experiments[key] // map[T]bool returns false for missing keys +// IsEnabled reports whether the named experiment is enabled in the context. +func IsEnabled(ctx context.Context, key string) bool { + state := ctx.Value(experimentCtxKey{key}) + return state != nil && state.(bool) } // Enabled returns the keys of all the enabled experiments. -func Enabled() []string { +func Enabled(ctx context.Context) []string { + allMu.Lock() + defer allMu.Unlock() var keys []string - for key, enabled := range experiments { - if enabled { + for key := range all { + if IsEnabled(ctx, key) { keys = append(keys, key) } } diff --git a/internal/job/api.go b/internal/job/api.go index 5f27eb5a3b..45a2535226 100644 --- a/internal/job/api.go +++ b/internal/job/api.go @@ -1,9 +1,10 @@ package job import ( + "context" "fmt" - "github.com/buildkite/agent/v3/experiments" + "github.com/buildkite/agent/v3/internal/experiments" "github.com/buildkite/agent/v3/internal/socket" "github.com/buildkite/agent/v3/jobapi" ) @@ -11,10 +12,10 @@ import ( // startJobAPI starts the job API server, iff the job API experiment is enabled, and the OS of the box supports it // otherwise it returns a noop cleanup function // It also sets the BUILDKITE_AGENT_JOB_API_SOCKET and BUILDKITE_AGENT_JOB_API_TOKEN environment variables -func (e *Executor) startJobAPI() (cleanup func(), err error) { +func (e *Executor) startJobAPI(ctx context.Context) (cleanup func(), err error) { cleanup = func() {} - if !experiments.IsEnabled(experiments.JobAPI) { + if !experiments.IsEnabled(ctx, experiments.JobAPI) { return cleanup, nil } diff --git a/internal/job/executor.go b/internal/job/executor.go index 2598fa0a6d..08430ffa33 100644 --- a/internal/job/executor.go +++ b/internal/job/executor.go @@ -20,8 +20,8 @@ import ( "github.com/buildkite/agent/v3/agent/plugin" "github.com/buildkite/agent/v3/env" - "github.com/buildkite/agent/v3/experiments" "github.com/buildkite/agent/v3/hook" + "github.com/buildkite/agent/v3/internal/experiments" "github.com/buildkite/agent/v3/internal/job/shell" "github.com/buildkite/agent/v3/internal/redact" "github.com/buildkite/agent/v3/internal/replacer" @@ -83,7 +83,7 @@ func (e *Executor) Run(ctx context.Context) (exitCode int) { e.shell.InterruptSignal = e.ExecutorConfig.CancelSignal e.shell.SignalGracePeriod = e.ExecutorConfig.SignalGracePeriod } - if experiments.IsEnabled(experiments.KubernetesExec) { + if experiments.IsEnabled(ctx, experiments.KubernetesExec) { kubernetesClient := &kubernetes.Client{} if err := e.startKubernetesClient(ctx, kubernetesClient); err != nil { e.shell.Errorf("Failed to start kubernetes client: %v", err) @@ -120,7 +120,7 @@ func (e *Executor) Run(ctx context.Context) (exitCode int) { e.shell.Env = env.FromSlice(os.Environ()) // Initialize the job API, iff the experiment is enabled. Noop otherwise - cleanup, err := e.startJobAPI() + cleanup, err := e.startJobAPI(ctx) if err != nil { e.shell.Errorf("Error setting up job API: %v", err) return 1 @@ -300,7 +300,7 @@ func (e *Executor) executeHook(ctx context.Context, hookCfg HookConfig) error { e.shell.Headerf("Running %s hook", hookName) - if !experiments.IsEnabled(experiments.PolyglotHooks) { + if !experiments.IsEnabled(ctx, experiments.PolyglotHooks) { return e.runWrappedShellScriptHook(ctx, hookName, hookCfg) } @@ -769,7 +769,7 @@ func (e *Executor) PluginPhase(ctx context.Context) error { } checkoutPluginMethod := e.checkoutPlugin - if experiments.IsEnabled(experiments.IsolatedPluginCheckout) { + if experiments.IsEnabled(ctx, experiments.IsolatedPluginCheckout) { if e.Debug { e.shell.Commentf("Using isolated plugin checkout") } @@ -1870,7 +1870,7 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) error { } // resolve BUILDKITE_COMMIT based on the local git repo - if experiments.IsEnabled(experiments.ResolveCommitAfterCheckout) { + if experiments.IsEnabled(ctx, experiments.ResolveCommitAfterCheckout) { e.shell.Commentf("Using resolve-commit-after-checkout experiment 🧪") e.resolveCommit(ctx) } @@ -2027,7 +2027,7 @@ func (e *Executor) CommandPhase(ctx context.Context) (hookErr error, commandErr isExitError := shell.IsExitError(commandErr) isExitSignaled := shell.IsExitSignaled(commandErr) - avoidRecursiveTrap := experiments.IsEnabled(experiments.AvoidRecursiveTrap) + avoidRecursiveTrap := experiments.IsEnabled(ctx, experiments.AvoidRecursiveTrap) switch { case isExitError && isExitSignaled && avoidRecursiveTrap: @@ -2190,7 +2190,7 @@ func (e *Executor) defaultCommandPhase(ctx context.Context) error { // elsewhere in the agent. // // Therefore, we are phasing it out under an experiment. - if !experiments.IsEnabled(experiments.AvoidRecursiveTrap) && !commandIsScript && shellscript.IsPOSIXShell(e.Shell) { + if !experiments.IsEnabled(ctx, experiments.AvoidRecursiveTrap) && !commandIsScript && shellscript.IsPOSIXShell(e.Shell) { cmdToExec = fmt.Sprintf("trap 'kill -- $$' INT TERM QUIT; %s", cmdToExec) } diff --git a/internal/job/integration/artifact_integration_test.go b/internal/job/integration/artifact_integration_test.go index e03e0f4d70..d73083366c 100644 --- a/internal/job/integration/artifact_integration_test.go +++ b/internal/job/integration/artifact_integration_test.go @@ -11,7 +11,7 @@ import ( func TestArtifactsUploadAfterCommand(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -40,7 +40,7 @@ func TestArtifactsUploadAfterCommand(t *testing.T) { func TestArtifactsUploadAfterCommandFails(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -74,7 +74,7 @@ func TestArtifactsUploadAfterCommandFails(t *testing.T) { func TestArtifactsUploadAfterCommandHookFails(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } diff --git a/internal/job/integration/checkout_git_mirrors_integration_test.go b/internal/job/integration/checkout_git_mirrors_integration_test.go index cf4aec5090..d7a028ad35 100644 --- a/internal/job/integration/checkout_git_mirrors_integration_test.go +++ b/internal/job/integration/checkout_git_mirrors_integration_test.go @@ -10,14 +10,14 @@ import ( "sync/atomic" "testing" - "github.com/buildkite/agent/v3/experiments" + "github.com/buildkite/agent/v3/internal/experiments" "github.com/buildkite/bintest/v3" ) func TestCheckingOutGitHubPullRequests_WithGitMirrors(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -59,10 +59,10 @@ func TestCheckingOutGitHubPullRequests_WithGitMirrors(t *testing.T) { } func TestWithResolvingCommitExperiment_WithGitMirrors(t *testing.T) { - // t.Parallel() cannot be used with experiments.Enable() - defer experiments.EnableWithUndo(experiments.ResolveCommitAfterCheckout)() + t.Parallel() - tester, err := NewBootstrapTester() + ctx, _ := experiments.Enable(mainCtx, experiments.ResolveCommitAfterCheckout) + tester, err := NewBootstrapTester(ctx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -107,7 +107,7 @@ func TestWithResolvingCommitExperiment_WithGitMirrors(t *testing.T) { func TestCheckingOutLocalGitProject_WithGitMirrors(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -156,7 +156,7 @@ func TestCheckingOutLocalGitProjectWithSubmodules_WithGitMirrors(t *testing.T) { t.Skip() } - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -228,7 +228,7 @@ func TestCheckingOutLocalGitProjectWithSubmodulesDisabled_WithGitMirrors(t *test t.Skip() } - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -289,7 +289,7 @@ func TestCheckingOutLocalGitProjectWithSubmodulesDisabled_WithGitMirrors(t *test func TestCheckingOutShallowCloneOfLocalGitProject_WithGitMirrors(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -333,7 +333,7 @@ func TestCheckingOutShallowCloneOfLocalGitProject_WithGitMirrors(t *testing.T) { func TestCheckingOutSetsCorrectGitMetadataAndSendsItToBuildkite_WithGitMirrors(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -353,7 +353,7 @@ func TestCheckingOutSetsCorrectGitMetadataAndSendsItToBuildkite_WithGitMirrors(t func TestCheckingOutWithSSHKeyscan_WithGitMirrors(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -385,7 +385,7 @@ func TestCheckingOutWithSSHKeyscan_WithGitMirrors(t *testing.T) { func TestCheckingOutWithoutSSHKeyscan_WithGitMirrors(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -410,7 +410,7 @@ func TestCheckingOutWithoutSSHKeyscan_WithGitMirrors(t *testing.T) { func TestCheckingOutWithSSHKeyscanAndUnscannableRepo_WithGitMirrors(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -443,7 +443,7 @@ func TestCleaningAnExistingCheckout_WithGitMirrors(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -481,7 +481,9 @@ func TestCleaningAnExistingCheckout_WithGitMirrors(t *testing.T) { } func TestForcingACleanCheckout_WithGitMirrors(t *testing.T) { - tester, err := NewBootstrapTester() + t.Parallel() + + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -505,7 +507,9 @@ func TestForcingACleanCheckout_WithGitMirrors(t *testing.T) { } func TestCheckoutOnAnExistingRepositoryWithoutAGitFolder_WithGitMirrors(t *testing.T) { - tester, err := NewBootstrapTester() + t.Parallel() + + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -534,7 +538,9 @@ func TestCheckoutOnAnExistingRepositoryWithoutAGitFolder_WithGitMirrors(t *testi } func TestCheckoutRetriesOnCleanFailure_WithGitMirrors(t *testing.T) { - tester, err := NewBootstrapTester() + t.Parallel() + + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -564,7 +570,9 @@ func TestCheckoutRetriesOnCleanFailure_WithGitMirrors(t *testing.T) { } func TestCheckoutRetriesOnCloneFailure_WithGitMirrors(t *testing.T) { - tester, err := NewBootstrapTester() + t.Parallel() + + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -592,7 +600,9 @@ func TestCheckoutRetriesOnCloneFailure_WithGitMirrors(t *testing.T) { } func TestCheckoutDoesNotRetryOnHookFailure_WithGitMirrors(t *testing.T) { - tester, err := NewBootstrapTester() + t.Parallel() + + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -629,7 +639,7 @@ func TestRepositorylessCheckout_WithGitMirrors(t *testing.T) { t.Skip("Not supported on windows") } - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -662,7 +672,7 @@ func TestRepositorylessCheckout_WithGitMirrors(t *testing.T) { func TestGitMirrorEnv(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } diff --git a/internal/job/integration/checkout_integration_test.go b/internal/job/integration/checkout_integration_test.go index ffc53f7294..8e2e757d64 100644 --- a/internal/job/integration/checkout_integration_test.go +++ b/internal/job/integration/checkout_integration_test.go @@ -12,7 +12,7 @@ import ( "sync/atomic" "testing" - "github.com/buildkite/agent/v3/experiments" + "github.com/buildkite/agent/v3/internal/experiments" "github.com/buildkite/bintest/v3" ) @@ -29,10 +29,10 @@ var commitPattern = bintest.MatchPattern(`(?ms)\Acommit [0-9a-f]+\nabbrev-commit const gitShowFormatArg = "--format=commit %H%nabbrev-commit %h%nAuthor: %an <%ae>%n%n%w(0,4,4)%B" func TestWithResolvingCommitExperiment(t *testing.T) { - // t.Parallel() cannot be used with experiments.Enable() - defer experiments.EnableWithUndo(experiments.ResolveCommitAfterCheckout)() + t.Parallel() - tester, err := NewBootstrapTester() + ctx, _ := experiments.Enable(mainCtx, experiments.ResolveCommitAfterCheckout) + tester, err := NewBootstrapTester(ctx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -72,7 +72,7 @@ func TestWithResolvingCommitExperiment(t *testing.T) { func TestCheckingOutLocalGitProject(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -116,7 +116,7 @@ func TestCheckingOutLocalGitProjectWithSubmodules(t *testing.T) { t.Skip() } - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -182,7 +182,7 @@ func TestCheckingOutLocalGitProjectWithSubmodulesDisabled(t *testing.T) { t.Skip() } - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -238,7 +238,7 @@ func TestCheckingOutLocalGitProjectWithSubmodulesDisabled(t *testing.T) { func TestCheckingOutShallowCloneOfLocalGitProject(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -277,7 +277,7 @@ func TestCheckingOutShallowCloneOfLocalGitProject(t *testing.T) { func TestCheckoutErrorIsRetried(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -340,7 +340,7 @@ func TestCheckoutErrorIsRetried(t *testing.T) { func TestFetchErrorIsRetried(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -402,7 +402,7 @@ func TestFetchErrorIsRetried(t *testing.T) { func TestCheckingOutSetsCorrectGitMetadataAndSendsItToBuildkite(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -418,7 +418,7 @@ func TestCheckingOutSetsCorrectGitMetadataAndSendsItToBuildkite(t *testing.T) { func TestCheckingOutWithSSHKeyscan(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -446,7 +446,7 @@ func TestCheckingOutWithSSHKeyscan(t *testing.T) { func TestCheckingOutWithoutSSHKeyscan(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -467,7 +467,7 @@ func TestCheckingOutWithoutSSHKeyscan(t *testing.T) { func TestCheckingOutWithSSHKeyscanAndUnscannableRepo(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -496,7 +496,7 @@ func TestCleaningAnExistingCheckout(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -530,7 +530,9 @@ func TestCleaningAnExistingCheckout(t *testing.T) { } func TestForcingACleanCheckout(t *testing.T) { - tester, err := NewBootstrapTester() + t.Parallel() + + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -550,7 +552,9 @@ func TestForcingACleanCheckout(t *testing.T) { } func TestCheckoutOnAnExistingRepositoryWithoutAGitFolder(t *testing.T) { - tester, err := NewBootstrapTester() + t.Parallel() + + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -575,7 +579,9 @@ func TestCheckoutOnAnExistingRepositoryWithoutAGitFolder(t *testing.T) { } func TestCheckoutRetriesOnCleanFailure(t *testing.T) { - tester, err := NewBootstrapTester() + t.Parallel() + + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -601,7 +607,9 @@ func TestCheckoutRetriesOnCleanFailure(t *testing.T) { } func TestCheckoutRetriesOnCloneFailure(t *testing.T) { - tester, err := NewBootstrapTester() + t.Parallel() + + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -625,7 +633,9 @@ func TestCheckoutRetriesOnCloneFailure(t *testing.T) { } func TestCheckoutDoesNotRetryOnHookFailure(t *testing.T) { - tester, err := NewBootstrapTester() + t.Parallel() + + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -658,7 +668,7 @@ func TestRepositorylessCheckout(t *testing.T) { t.Skip("Not supported on windows") } - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } diff --git a/internal/job/integration/command_integration_test.go b/internal/job/integration/command_integration_test.go index e63b6d06ee..b483dc30e0 100644 --- a/internal/job/integration/command_integration_test.go +++ b/internal/job/integration/command_integration_test.go @@ -14,7 +14,7 @@ func TestMultilineCommandRunUnderBatch(t *testing.T) { t.Skip("batch test only applies to Windows") } - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -44,7 +44,7 @@ func TestMultilineCommandRunUnderBatch(t *testing.T) { func TestPreExitHooksRunsAfterCommandFails(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } diff --git a/internal/job/integration/docker_integration_test.go b/internal/job/integration/docker_integration_test.go index 2c204a609d..87f3d234b0 100644 --- a/internal/job/integration/docker_integration_test.go +++ b/internal/job/integration/docker_integration_test.go @@ -17,7 +17,7 @@ func argumentForCommand(cmd string) any { } func TestRunningCommandWithDocker(t *testing.T) { - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -50,7 +50,7 @@ func TestRunningCommandWithDocker(t *testing.T) { } func TestRunningCommandWithDockerAndCustomDockerfile(t *testing.T) { - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -84,7 +84,7 @@ func TestRunningCommandWithDockerAndCustomDockerfile(t *testing.T) { } func TestRunningFailingCommandWithDocker(t *testing.T) { - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -123,7 +123,7 @@ func TestRunningFailingCommandWithDocker(t *testing.T) { } func TestRunningCommandWithDockerCompose(t *testing.T) { - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -156,7 +156,7 @@ func TestRunningCommandWithDockerCompose(t *testing.T) { } func TestRunningFailingCommandWithDockerCompose(t *testing.T) { - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -196,7 +196,7 @@ func TestRunningFailingCommandWithDockerCompose(t *testing.T) { } func TestRunningCommandWithDockerComposeAndExtraConfig(t *testing.T) { - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -230,7 +230,7 @@ func TestRunningCommandWithDockerComposeAndExtraConfig(t *testing.T) { } func TestRunningCommandWithDockerComposeAndBuildAll(t *testing.T) { - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } diff --git a/internal/job/integration/executor_tester.go b/internal/job/integration/executor_tester.go index c6a15459c0..630775ea63 100644 --- a/internal/job/integration/executor_tester.go +++ b/internal/job/integration/executor_tester.go @@ -3,6 +3,7 @@ package integration import ( "bufio" "bytes" + "context" "encoding/json" "fmt" "io" @@ -18,7 +19,7 @@ import ( "testing" "github.com/buildkite/agent/v3/env" - "github.com/buildkite/agent/v3/experiments" + "github.com/buildkite/agent/v3/internal/experiments" "github.com/buildkite/bintest/v3" ) @@ -43,7 +44,7 @@ type ExecutorTester struct { mocks []*bintest.Mock } -func NewBootstrapTester() (*ExecutorTester, error) { +func NewBootstrapTester(ctx context.Context) (*ExecutorTester, error) { // The job API experiment adds a unix domain socket to a directory in the home directory // UDS names are limited to 108 characters, so we need to use a shorter home directory // Who knows what's going on in windowsland @@ -114,7 +115,7 @@ func NewBootstrapTester() (*ExecutorTester, error) { } // Support testing experiments - experiments := experiments.Enabled() + experiments := experiments.Enabled(ctx) if len(experiments) > 0 { bt.Env = append(bt.Env, "BUILDKITE_AGENT_EXPERIMENT="+strings.Join(experiments, ",")) } diff --git a/internal/job/integration/hooks_integration_test.go b/internal/job/integration/hooks_integration_test.go index 8d726b790b..0f922d86a4 100644 --- a/internal/job/integration/hooks_integration_test.go +++ b/internal/job/integration/hooks_integration_test.go @@ -11,7 +11,7 @@ import ( "testing" "time" - "github.com/buildkite/agent/v3/experiments" + "github.com/buildkite/agent/v3/internal/experiments" "github.com/buildkite/agent/v3/internal/job/shell" "github.com/buildkite/bintest/v3" ) @@ -19,7 +19,7 @@ import ( func TestEnvironmentVariablesPassBetweenHooks(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -63,7 +63,7 @@ func TestEnvironmentVariablesPassBetweenHooks(t *testing.T) { func TestHooksCanUnsetEnvironmentVariables(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -123,7 +123,7 @@ func TestHooksCanUnsetEnvironmentVariables(t *testing.T) { func TestDirectoryPassesBetweenHooks(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -157,7 +157,7 @@ func TestDirectoryPassesBetweenHooks(t *testing.T) { } func TestDirectoryPassesBetweenHooksIgnoredUnderExit(t *testing.T) { - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -194,7 +194,7 @@ func TestDirectoryPassesBetweenHooksIgnoredUnderExit(t *testing.T) { func TestCheckingOutFiresCorrectHooks(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -224,7 +224,7 @@ func TestCheckingOutFiresCorrectHooks(t *testing.T) { func TestReplacingCheckoutHook(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -253,7 +253,7 @@ func TestReplacingCheckoutHook(t *testing.T) { func TestReplacingGlobalCommandHook(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -278,7 +278,7 @@ func TestReplacingGlobalCommandHook(t *testing.T) { func TestReplacingLocalCommandHook(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -304,7 +304,7 @@ func TestReplacingLocalCommandHook(t *testing.T) { func TestPreExitHooksFireAfterCommandFailures(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -323,29 +323,86 @@ func TestPreExitHooksFireAfterCommandFailures(t *testing.T) { func TestPreExitHooksFireAfterHookFailures(t *testing.T) { t.Parallel() - var testCases = []struct { + ctx := mainCtx + + testCases := []struct { failingHook string expectGlobalPreExit bool expectLocalPreExit bool expectCheckout bool expectArtifacts bool }{ - {"environment", true, false, false, false}, - {"pre-checkout", true, false, false, false}, - {"post-checkout", true, true, true, true}, - {"checkout", true, false, false, false}, - {"pre-command", true, true, true, true}, - {"command", true, true, true, true}, - {"post-command", true, true, true, true}, - {"pre-artifact", true, true, true, false}, - {"post-artifact", true, true, true, true}, + { + failingHook: "environment", + expectGlobalPreExit: true, + expectLocalPreExit: false, + expectCheckout: false, + expectArtifacts: false, + }, + { + failingHook: "pre-checkout", + expectGlobalPreExit: true, + expectLocalPreExit: false, + expectCheckout: false, + expectArtifacts: false, + }, + { + failingHook: "post-checkout", + expectGlobalPreExit: true, + expectLocalPreExit: true, + expectCheckout: true, + expectArtifacts: false, + }, + { + failingHook: "checkout", + expectGlobalPreExit: true, + expectLocalPreExit: false, + expectCheckout: false, + expectArtifacts: false, + }, + { + failingHook: "pre-command", + expectGlobalPreExit: true, + expectLocalPreExit: true, + expectCheckout: true, + expectArtifacts: true, + }, + { + failingHook: "command", + expectGlobalPreExit: true, + expectLocalPreExit: true, + expectCheckout: true, + expectArtifacts: true, + }, + { + failingHook: "post-command", + expectGlobalPreExit: true, + expectLocalPreExit: true, + expectCheckout: true, + expectArtifacts: true, + }, + { + failingHook: "pre-artifact", + expectGlobalPreExit: true, + expectLocalPreExit: true, + expectCheckout: true, + expectArtifacts: false, + }, + { + failingHook: "post-artifact", + expectGlobalPreExit: true, + expectLocalPreExit: true, + expectCheckout: true, + expectArtifacts: true, + }, } for _, tc := range testCases { + tc := tc t.Run(tc.failingHook, func(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(ctx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -395,7 +452,7 @@ func TestPreExitHooksFireAfterHookFailures(t *testing.T) { func TestNoLocalHooksCalledWhenConfigSet(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -416,6 +473,8 @@ func TestNoLocalHooksCalledWhenConfigSet(t *testing.T) { func TestExitCodesPropagateOutFromGlobalHooks(t *testing.T) { t.Parallel() + ctx := mainCtx + for _, hook := range []string{ "environment", "pre-checkout", @@ -429,7 +488,7 @@ func TestExitCodesPropagateOutFromGlobalHooks(t *testing.T) { // "post-artifact", } { t.Run(hook, func(t *testing.T) { - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(ctx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -452,14 +511,14 @@ func TestExitCodesPropagateOutFromGlobalHooks(t *testing.T) { } func TestPreExitHooksFireAfterCancel(t *testing.T) { + t.Parallel() + // TODO: Why is this test skipped on windows and darwin? if runtime.GOOS == "windows" || runtime.GOOS == "darwin" { t.Skip() } - t.Parallel() - - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -489,6 +548,8 @@ func TestPreExitHooksFireAfterCancel(t *testing.T) { } func TestPolyglotScriptHooksCanBeRun(t *testing.T) { + t.Parallel() + if runtime.GOOS == "windows" { t.Skip("script hooks aren't supported on windows") } @@ -502,9 +563,9 @@ func TestPolyglotScriptHooksCanBeRun(t *testing.T) { t.Fatal("ruby not found in $PATH. This test requires ruby to be installed on the host") } - defer experiments.EnableWithUndo(experiments.PolyglotHooks)() + ctx, _ := experiments.Enable(mainCtx, experiments.PolyglotHooks) - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(ctx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -528,10 +589,13 @@ func TestPolyglotScriptHooksCanBeRun(t *testing.T) { } func TestPolyglotBinaryHooksCanBeRun(t *testing.T) { - defer experiments.EnableWithUndo(experiments.PolyglotHooks)() - defer experiments.EnableWithUndo(experiments.JobAPI)() + t.Parallel() + + ctx := mainCtx + ctx, _ = experiments.Enable(ctx, experiments.PolyglotHooks) + ctx, _ = experiments.Enable(ctx, experiments.JobAPI) - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(ctx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } diff --git a/internal/job/integration/isolated_plugin_integration_test.go b/internal/job/integration/isolated_plugin_integration_test.go index 701054fff4..f14efc8bc0 100644 --- a/internal/job/integration/isolated_plugin_integration_test.go +++ b/internal/job/integration/isolated_plugin_integration_test.go @@ -6,7 +6,7 @@ import ( "runtime" "testing" - "github.com/buildkite/agent/v3/experiments" + "github.com/buildkite/agent/v3/internal/experiments" "github.com/buildkite/bintest/v3" ) @@ -19,12 +19,11 @@ import ( // that a plugin modified upstream is treated as expected. That is, by default, the updates won't // take effect, but with BUILDKITE_PLUGINS_ALWAYS_CLONE_FRESH set, they will. func TestModifiedIsolatedPluginNoForcePull(t *testing.T) { - // t.Parallel() cannot be used here because we are modifying the global experiments state + t.Parallel() - undo := experiments.EnableWithUndo(experiments.IsolatedPluginCheckout) - defer undo() + ctx, _ := experiments.Enable(mainCtx, experiments.IsolatedPluginCheckout) - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(ctx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -89,7 +88,7 @@ func TestModifiedIsolatedPluginNoForcePull(t *testing.T) { tester.RunAndCheck(t, env...) // Now, we want to "repeat" the test build, having modified the plugin's contents. - tester2, err := NewBootstrapTester() + tester2, err := NewBootstrapTester(ctx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -131,12 +130,11 @@ func TestModifiedIsolatedPluginNoForcePull(t *testing.T) { // and after modifying a plugin's source, but this time with BUILDKITE_PLUGINS_ALWAYS_CLONE_FRESH // set to true. So, we expect the upstream plugin changes to take effect on our second build. func TestModifiedIsolatedPluginWithForcePull(t *testing.T) { - // t.Parallel() cannot be used here because we are modifying the global experiments state + t.Parallel() - undo := experiments.EnableWithUndo(experiments.IsolatedPluginCheckout) - defer undo() + ctx, _ := experiments.Enable(mainCtx, experiments.IsolatedPluginCheckout) - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(ctx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -193,7 +191,7 @@ func TestModifiedIsolatedPluginWithForcePull(t *testing.T) { tester.RunAndCheck(t, env...) - tester2, err := NewBootstrapTester() + tester2, err := NewBootstrapTester(ctx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } diff --git a/internal/job/integration/job_api_integration_test.go b/internal/job/integration/job_api_integration_test.go index 8c834e0fb6..5f26d3740b 100644 --- a/internal/job/integration/job_api_integration_test.go +++ b/internal/job/integration/job_api_integration_test.go @@ -10,15 +10,17 @@ import ( "net/http" "testing" - "github.com/buildkite/agent/v3/experiments" + "github.com/buildkite/agent/v3/internal/experiments" "github.com/buildkite/agent/v3/jobapi" "github.com/buildkite/bintest/v3" ) func TestBootstrapRunsJobAPI(t *testing.T) { - defer experiments.EnableWithUndo(experiments.JobAPI)() + t.Parallel() - tester, err := NewBootstrapTester() + ctx, _ := experiments.Enable(mainCtx, experiments.JobAPI) + + tester, err := NewBootstrapTester(ctx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } diff --git a/internal/job/integration/main_test.go b/internal/job/integration/main_test.go index b55c57b0e9..3840f25a13 100644 --- a/internal/job/integration/main_test.go +++ b/internal/job/integration/main_test.go @@ -1,17 +1,23 @@ package integration import ( + "context" "fmt" "os" "testing" "github.com/buildkite/agent/v3/clicommand" - "github.com/buildkite/agent/v3/experiments" + "github.com/buildkite/agent/v3/internal/experiments" "github.com/buildkite/agent/v3/version" "github.com/buildkite/bintest/v3" "github.com/urfave/cli" ) +// This context needs to be stored here in order to pass experiments to tests, +// because testing.M can only Run (without passing arguments or context). +// In ordinary code, pass contexts as arguments! +var mainCtx = context.Background() + func TestMain(m *testing.M) { // If we are passed "bootstrap", execute like the bootstrap cli if len(os.Args) > 1 && os.Args[1] == "bootstrap" { @@ -36,7 +42,7 @@ func TestMain(m *testing.M) { // Support running the test suite against a given experiment if exp := os.Getenv("TEST_EXPERIMENT"); exp != "" { - experiments.Enable(exp) + mainCtx, _ = experiments.Enable(mainCtx, exp) fmt.Printf("!!! Enabling experiment %q for test suite\n", exp) } diff --git a/internal/job/integration/plugin_integration_test.go b/internal/job/integration/plugin_integration_test.go index 5f62b31f4e..e129a10194 100644 --- a/internal/job/integration/plugin_integration_test.go +++ b/internal/job/integration/plugin_integration_test.go @@ -17,7 +17,7 @@ import ( func TestRunningPlugins(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -78,7 +78,7 @@ func TestRunningPlugins(t *testing.T) { func TestExitCodesPropagateOutFromPlugins(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -125,7 +125,7 @@ func TestExitCodesPropagateOutFromPlugins(t *testing.T) { func TestMalformedPluginNamesDontCrashBootstrap(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -149,7 +149,7 @@ func TestMalformedPluginNamesDontCrashBootstrap(t *testing.T) { func TestOverlappingPluginHooks(t *testing.T) { t.Parallel() - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -207,13 +207,13 @@ func TestOverlappingPluginHooks(t *testing.T) { } func TestPluginCloneRetried(t *testing.T) { + t.Parallel() + if runtime.GOOS == "windows" { t.Skip("Not passing on windows, needs investigation") } - t.Parallel() - - tester, err := NewBootstrapTester() + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -278,7 +278,11 @@ func TestPluginCloneRetried(t *testing.T) { // that a plugin modified upstream is treated as expected. That is, by default, the updates won't // take effect, but with BUILDKITE_PLUGINS_ALWAYS_CLONE_FRESH set, they will. func TestModifiedPluginNoForcePull(t *testing.T) { - tester, err := NewBootstrapTester() + t.Parallel() + + ctx := mainCtx + + tester, err := NewBootstrapTester(ctx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -343,7 +347,7 @@ func TestModifiedPluginNoForcePull(t *testing.T) { tester.RunAndCheck(t, env...) // Now, we want to "repeat" the test build, having modified the plugin's contents. - tester2, err := NewBootstrapTester() + tester2, err := NewBootstrapTester(ctx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -385,7 +389,11 @@ func TestModifiedPluginNoForcePull(t *testing.T) { // and after modifying a plugin's source, but this time with BUILDKITE_PLUGINS_ALWAYS_CLONE_FRESH // set to true. So, we expect the upstream plugin changes to take effect on our second build. func TestModifiedPluginWithForcePull(t *testing.T) { - tester, err := NewBootstrapTester() + t.Parallel() + + ctx := mainCtx + + tester, err := NewBootstrapTester(mainCtx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) } @@ -442,7 +450,7 @@ func TestModifiedPluginWithForcePull(t *testing.T) { tester.RunAndCheck(t, env...) - tester2, err := NewBootstrapTester() + tester2, err := NewBootstrapTester(ctx) if err != nil { t.Fatalf("NewBootstrapTester() error = %v", err) }