From faa40d5fa91ba5e6adfbc1dcece64b25f45bb36f Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Wed, 18 Oct 2023 09:28:50 -0700 Subject: [PATCH 1/6] [services] Fix issue where env var is ignored --- devbox.go | 10 ++--- internal/boxcli/run.go | 17 ++------ internal/boxcli/services.go | 27 +++++++++--- internal/impl/devbox.go | 70 +++++++++++++++++++----------- internal/impl/devopt/devboxopts.go | 1 - internal/impl/envpath/pathlists.go | 20 +++++++++ internal/wrapnix/wrapper.go | 4 +- 7 files changed, 97 insertions(+), 52 deletions(-) diff --git a/devbox.go b/devbox.go index fc911dc1a0c..de771c4cc22 100644 --- a/devbox.go +++ b/devbox.go @@ -34,12 +34,12 @@ type Devbox interface { Update(ctx context.Context, opts devopt.UpdateOpts) error // Interact with services - ListServices(ctx context.Context) error - RestartServices(ctx context.Context, services ...string) error + ListServices(ctx context.Context, runInCurrentShell bool) error + RestartServices(ctx context.Context, runInCurrentShell bool, services ...string) error Services() (services.Services, error) - StartProcessManager(ctx context.Context, requestedServices []string, background bool, processComposeFileOrDir string) error - StartServices(ctx context.Context, services ...string) error - StopServices(ctx context.Context, allProjects bool, services ...string) error + StartProcessManager(ctx context.Context, runInCurrentShell bool, requestedServices []string, background bool, processComposeFileOrDir string) error + StartServices(ctx context.Context, runInCurrentShell bool, services ...string) error + StopServices(ctx context.Context, runInCurrentShell bool, allProjects bool, services ...string) error // Generate files Generate(ctx context.Context) error diff --git a/internal/boxcli/run.go b/internal/boxcli/run.go index cac3ac746eb..0f625e1f3e8 100644 --- a/internal/boxcli/run.go +++ b/internal/boxcli/run.go @@ -5,9 +5,7 @@ package boxcli import ( "fmt" - "os" "slices" - "strconv" "strings" "github.com/samber/lo" @@ -99,19 +97,12 @@ func runScriptCmd(cmd *cobra.Command, args []string, flags runCmdFlags) error { return err } - omitBinWrappersFromPath := true - // This is for testing. Once we completely remove bin wrappers we can remove - // this. It helps simulate shell using "run". - if ok, _ := strconv.ParseBool(os.Getenv("DEVBOX_INCLUDE_BIN_WRAPPERS_IN_PATH")); ok { - omitBinWrappersFromPath = false - } // Check the directory exists. box, err := devbox.Open(&devopt.Opts{ - Dir: path, - Stderr: cmd.ErrOrStderr(), - Pure: flags.pure, - Env: env, - OmitBinWrappersFromPath: omitBinWrappersFromPath, + Dir: path, + Stderr: cmd.ErrOrStderr(), + Pure: flags.pure, + Env: env, }) if err != nil { return redact.Errorf("error reading devbox.json: %w", err) diff --git a/internal/boxcli/services.go b/internal/boxcli/services.go index c19be2f809f..9f05257e064 100644 --- a/internal/boxcli/services.go +++ b/internal/boxcli/services.go @@ -12,7 +12,8 @@ import ( type servicesCmdFlags struct { envFlag - config configFlags + config configFlags + runInCurrentShell bool } type serviceUpFlags struct { @@ -105,6 +106,13 @@ func servicesCmd(persistentPreRunE ...cobraFunc) *cobra.Command { flags.envFlag.register(servicesCommand) flags.config.registerPersistent(servicesCommand) + servicesCommand.PersistentFlags().BoolVar( + &flags.runInCurrentShell, + "run-in-current-shell", + false, + "Run the command in the current shell instead of a new shell", + ) + servicesCommand.Flag("run-in-current-shell").Hidden = true serviceUpFlags.register(upCommand) serviceStopFlags.register(stopCommand) servicesCommand.AddCommand(lsCommand) @@ -124,7 +132,7 @@ func listServices(cmd *cobra.Command, flags servicesCmdFlags) error { return errors.WithStack(err) } - return box.ListServices(cmd.Context()) + return box.ListServices(cmd.Context(), flags.runInCurrentShell) } func startServices(cmd *cobra.Command, services []string, flags servicesCmdFlags) error { @@ -141,7 +149,7 @@ func startServices(cmd *cobra.Command, services []string, flags servicesCmdFlags return errors.WithStack(err) } - return box.StartServices(cmd.Context(), services...) + return box.StartServices(cmd.Context(), flags.runInCurrentShell, services...) } func stopServices( @@ -165,7 +173,8 @@ func stopServices( if len(services) > 0 && flags.allProjects { return errors.New("cannot use both services and --all-projects arguments simultaneously") } - return box.StopServices(cmd.Context(), flags.allProjects, services...) + return box.StopServices( + cmd.Context(), servicesFlags.runInCurrentShell, flags.allProjects, services...) } func restartServices( @@ -186,7 +195,7 @@ func restartServices( return errors.WithStack(err) } - return box.RestartServices(cmd.Context(), services...) + return box.RestartServices(cmd.Context(), flags.runInCurrentShell, services...) } func startProcessManager( @@ -209,5 +218,11 @@ func startProcessManager( return errors.WithStack(err) } - return box.StartProcessManager(cmd.Context(), args, flags.background, flags.processComposeFile) + return box.StartProcessManager( + cmd.Context(), + servicesFlags.runInCurrentShell, + args, + flags.background, + flags.processComposeFile, + ) } diff --git a/internal/impl/devbox.go b/internal/impl/devbox.go index 952b59be5e7..de0b90a4ed1 100644 --- a/internal/impl/devbox.go +++ b/internal/impl/devbox.go @@ -30,6 +30,7 @@ import ( "go.jetpack.io/devbox/internal/searcher" "go.jetpack.io/devbox/internal/shellgen" "go.jetpack.io/devbox/internal/telemetry" + "go.jetpack.io/devbox/internal/wrapnix" "go.jetpack.io/devbox/internal/boxcli/usererr" "go.jetpack.io/devbox/internal/cmdutil" @@ -67,7 +68,6 @@ type Devbox struct { pure bool allowInsecureAdds bool customProcessComposeFile string - OmitBinWrappersFromPath bool // This is needed because of the --quiet flag. stderr io.Writer @@ -98,7 +98,6 @@ func Open(opts *devopt.Opts) (*Devbox, error) { pure: opts.Pure, customProcessComposeFile: opts.CustomProcessComposeFile, allowInsecureAdds: opts.AllowInsecureAdds, - OmitBinWrappersFromPath: opts.OmitBinWrappersFromPath, } lock, err := lock.GetFile(box) @@ -221,6 +220,19 @@ func (d *Devbox) RunScript(ctx context.Context, cmdName string, cmdArgs []string if err != nil { return err } + + // By default we always remove bin wrappers when using `run`. This env var is + // for testing. Once we completely remove bin wrappers we can remove this. + // It helps simulate shell using "run". + if includeBinWrappers, _ := strconv.ParseBool( + os.Getenv("DEVBOX_INCLUDE_BIN_WRAPPERS_IN_PATH"), + ); !includeBinWrappers { + env["PATH"] = envpath.RemoveFromPath( + env["PATH"], + wrapnix.WrapperBinPath(d.projectDir), + ) + } + // Used to determine whether we're inside a shell (e.g. to prevent shell inception) // This is temporary because StartServices() needs it but should be replaced with // better alternative since devbox run and devbox shell are not the same. @@ -526,15 +538,21 @@ func (d *Devbox) Services() (services.Services, error) { return result, nil } -func (d *Devbox) StartServices(ctx context.Context, serviceNames ...string) error { - if !d.IsEnvEnabled() { - return d.RunScript(ctx, "devbox", append([]string{"services", "start"}, serviceNames...)) +func (d *Devbox) StartServices( + ctx context.Context, runInCurrentShell bool, serviceNames ...string) error { + if !runInCurrentShell { + return d.RunScript(ctx, "devbox", + append( + []string{"services", "start", "--run-in-current-shell"}, + serviceNames..., + ), + ) } if !services.ProcessManagerIsRunning(d.projectDir) { fmt.Fprintln(d.stderr, "Process-compose is not running. Starting it now...") fmt.Fprintln(d.stderr, "\nNOTE: We recommend using `devbox services up` to start process-compose and your services") - return d.StartProcessManager(ctx, serviceNames, true, "") + return d.StartProcessManager(ctx, runInCurrentShell, serviceNames, true, "") } svcSet, err := d.Services() @@ -563,9 +581,9 @@ func (d *Devbox) StartServices(ctx context.Context, serviceNames ...string) erro return nil } -func (d *Devbox) StopServices(ctx context.Context, allProjects bool, serviceNames ...string) error { - if !d.IsEnvEnabled() { - args := []string{"services", "stop"} +func (d *Devbox) StopServices(ctx context.Context, runInCurrentShell bool, allProjects bool, serviceNames ...string) error { + if !runInCurrentShell { + args := []string{"services", "stop", "--run-in-current-shell"} args = append(args, serviceNames...) if allProjects { args = append(args, "--all-projects") @@ -602,9 +620,10 @@ func (d *Devbox) StopServices(ctx context.Context, allProjects bool, serviceName return nil } -func (d *Devbox) ListServices(ctx context.Context) error { - if !d.IsEnvEnabled() { - return d.RunScript(ctx, "devbox", []string{"services", "ls"}) +func (d *Devbox) ListServices(ctx context.Context, runInCurrentShell bool) error { + if !runInCurrentShell { + return d.RunScript(ctx, + "devbox", []string{"services", "ls", "--run-in-current-shell"}) } svcSet, err := d.Services() @@ -640,15 +659,21 @@ func (d *Devbox) ListServices(ctx context.Context) error { return nil } -func (d *Devbox) RestartServices(ctx context.Context, serviceNames ...string) error { - if !d.IsEnvEnabled() { - return d.RunScript(ctx, "devbox", append([]string{"services", "restart"}, serviceNames...)) +func (d *Devbox) RestartServices( + ctx context.Context, runInCurrentShell bool, serviceNames ...string) error { + if !runInCurrentShell { + return d.RunScript(ctx, "devbox", + append( + []string{"services", "restart", "--run-in-current-shell"}, + serviceNames..., + ), + ) } if !services.ProcessManagerIsRunning(d.projectDir) { fmt.Fprintln(d.stderr, "Process-compose is not running. Starting it now...") fmt.Fprintln(d.stderr, "\nTip: We recommend using `devbox services up` to start process-compose and your services") - return d.StartProcessManager(ctx, serviceNames, true, "") + return d.StartProcessManager(ctx, runInCurrentShell, serviceNames, true, "") } // TODO: Restart with no services should restart the _currently running_ services. This means we should get the list of running services from the process-compose, then restart them all. @@ -674,12 +699,13 @@ func (d *Devbox) RestartServices(ctx context.Context, serviceNames ...string) er func (d *Devbox) StartProcessManager( ctx context.Context, + runInCurrentShell bool, requestedServices []string, background bool, processComposeFileOrDir string, ) error { - if !d.IsEnvEnabled() { - args := []string{"services", "up"} + if !runInCurrentShell { + args := []string{"services", "up", "--run-in-current-shell"} args = append(args, requestedServices...) if processComposeFileOrDir != "" { args = append(args, "--process-compose-file", processComposeFileOrDir) @@ -853,17 +879,11 @@ func (d *Devbox) computeNixEnv(ctx context.Context, usePrintDevEnvCache bool) (m addEnvIfNotPreviouslySetByDevbox(env, pluginEnv) env["PATH"] = envpath.JoinPathLists( + filepath.Join(d.projectDir, plugin.WrapperBinPath), nix.ProfileBinPath(d.projectDir), env["PATH"], ) - if !d.OmitBinWrappersFromPath { - env["PATH"] = envpath.JoinPathLists( - filepath.Join(d.projectDir, plugin.WrapperBinPath), - env["PATH"], - ) - } - env["PATH"], err = d.addUtilitiesToPath(ctx, env["PATH"]) if err != nil { return nil, err diff --git a/internal/impl/devopt/devboxopts.go b/internal/impl/devopt/devboxopts.go index bdd916b4e9e..ce4cb75b7e1 100644 --- a/internal/impl/devopt/devboxopts.go +++ b/internal/impl/devopt/devboxopts.go @@ -12,7 +12,6 @@ type Opts struct { Pure bool IgnoreWarnings bool CustomProcessComposeFile string - OmitBinWrappersFromPath bool Stderr io.Writer } diff --git a/internal/impl/envpath/pathlists.go b/internal/impl/envpath/pathlists.go index a432a73807e..6554248c96a 100644 --- a/internal/impl/envpath/pathlists.go +++ b/internal/impl/envpath/pathlists.go @@ -1,6 +1,7 @@ package envpath import ( + "os" "path/filepath" "strings" ) @@ -35,3 +36,22 @@ func JoinPathLists(pathLists ...string) string { } return strings.Join(cleaned, string(filepath.ListSeparator)) } + +func RemoveFromPath(path, pathToRemove string) string { + paths := strings.Split(path, string(os.PathListSeparator)) + + // Create a new slice to store the modified paths + var newPaths []string + + // Iterate through the paths and add them to the newPaths slice if they are not equal to pathToRemove + for _, p := range paths { + if p != pathToRemove { + newPaths = append(newPaths, p) + } + } + + // Join the modified paths using ":" as the delimiter + newPath := strings.Join(newPaths, string(os.PathListSeparator)) + + return newPath +} diff --git a/internal/wrapnix/wrapper.go b/internal/wrapnix/wrapper.go index 1dec1b85860..024b5e4a61b 100644 --- a/internal/wrapnix/wrapper.go +++ b/internal/wrapnix/wrapper.go @@ -44,7 +44,7 @@ func CreateWrappers(ctx context.Context, args CreateWrappersArgs) error { _ = os.RemoveAll(filepath.Join(args.ProjectDir, plugin.WrapperPath)) // Recreate the bin wrapper directory - destPath := filepath.Join(wrapperBinPath(args.ProjectDir)) + destPath := filepath.Join(WrapperBinPath(args.ProjectDir)) _ = os.MkdirAll(destPath, 0o755) bashPath := cmdutil.GetPathOrDefault("bash", "/bin/bash") @@ -195,6 +195,6 @@ func createSymlinksForSupportDirs(projectDir string) error { return nil } -func wrapperBinPath(projectDir string) string { +func WrapperBinPath(projectDir string) string { return filepath.Join(projectDir, plugin.WrapperBinPath) } From 59edf11c9d243feb92f0d1eb2487cca72334cc07 Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Wed, 18 Oct 2023 09:42:54 -0700 Subject: [PATCH 2/6] Linter fixes --- devbox.go | 2 +- internal/impl/devbox.go | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/devbox.go b/devbox.go index de771c4cc22..e9e398c6baa 100644 --- a/devbox.go +++ b/devbox.go @@ -39,7 +39,7 @@ type Devbox interface { Services() (services.Services, error) StartProcessManager(ctx context.Context, runInCurrentShell bool, requestedServices []string, background bool, processComposeFileOrDir string) error StartServices(ctx context.Context, runInCurrentShell bool, services ...string) error - StopServices(ctx context.Context, runInCurrentShell bool, allProjects bool, services ...string) error + StopServices(ctx context.Context, runInCurrentShell, allProjects bool, services ...string) error // Generate files Generate(ctx context.Context) error diff --git a/internal/impl/devbox.go b/internal/impl/devbox.go index de0b90a4ed1..04de1378ccb 100644 --- a/internal/impl/devbox.go +++ b/internal/impl/devbox.go @@ -539,7 +539,8 @@ func (d *Devbox) Services() (services.Services, error) { } func (d *Devbox) StartServices( - ctx context.Context, runInCurrentShell bool, serviceNames ...string) error { + ctx context.Context, runInCurrentShell bool, serviceNames ...string, +) error { if !runInCurrentShell { return d.RunScript(ctx, "devbox", append( @@ -581,7 +582,7 @@ func (d *Devbox) StartServices( return nil } -func (d *Devbox) StopServices(ctx context.Context, runInCurrentShell bool, allProjects bool, serviceNames ...string) error { +func (d *Devbox) StopServices(ctx context.Context, runInCurrentShell, allProjects bool, serviceNames ...string) error { if !runInCurrentShell { args := []string{"services", "stop", "--run-in-current-shell"} args = append(args, serviceNames...) @@ -660,7 +661,8 @@ func (d *Devbox) ListServices(ctx context.Context, runInCurrentShell bool) error } func (d *Devbox) RestartServices( - ctx context.Context, runInCurrentShell bool, serviceNames ...string) error { + ctx context.Context, runInCurrentShell bool, serviceNames ...string, +) error { if !runInCurrentShell { return d.RunScript(ctx, "devbox", append( From ecf452adfa1543b210ffccff59f6b616dddea128 Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Wed, 18 Oct 2023 10:31:28 -0700 Subject: [PATCH 3/6] Fix test --- examples/stacks/lepp-stack/devbox.json | 4 +++- examples/stacks/lepp-stack/devbox.lock | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/examples/stacks/lepp-stack/devbox.json b/examples/stacks/lepp-stack/devbox.json index a389772f7c1..64dbe27fc2a 100644 --- a/examples/stacks/lepp-stack/devbox.json +++ b/examples/stacks/lepp-stack/devbox.json @@ -9,7 +9,8 @@ "env": { "NGINX_WEB_PORT": "8089", "NGINX_WEB_ROOT": "../../../my_app", - "PGPORT": "5433" + "PGPORT": "5433", + "PGHOST": "/tmp/devbox/lepp" }, "shell": { "scripts": { @@ -22,6 +23,7 @@ "run_test": [ "mkdir -p /tmp/devbox/lepp", "export PGHOST=/tmp/devbox/lepp", + "rm -rf .devbox/virtenv/postgresql/data", "initdb", "devbox services up -b", "echo 'sleep 2 second for the postgres server to initialize.' && sleep 2", diff --git a/examples/stacks/lepp-stack/devbox.lock b/examples/stacks/lepp-stack/devbox.lock index efcedd6d9c5..5e38e8e6001 100644 --- a/examples/stacks/lepp-stack/devbox.lock +++ b/examples/stacks/lepp-stack/devbox.lock @@ -22,7 +22,7 @@ }, "nginx@latest": { "last_modified": "2023-09-04T16:24:30Z", - "plugin_version": "0.0.3", + "plugin_version": "0.0.4", "resolved": "github:NixOS/nixpkgs/3c15feef7770eb5500a4b8792623e2d6f598c9c1#nginx", "source": "devbox-search", "version": "1.24.0", From 46499061c8fb008843f0bc2369462e8f344919ac Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Wed, 18 Oct 2023 11:27:48 -0700 Subject: [PATCH 4/6] Add docs --- internal/boxcli/services.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/internal/boxcli/services.go b/internal/boxcli/services.go index 9f05257e064..17fdebcb8b5 100644 --- a/internal/boxcli/services.go +++ b/internal/boxcli/services.go @@ -48,7 +48,13 @@ func servicesCmd(persistentPreRunE ...cobraFunc) *cobra.Command { serviceStopFlags := serviceStopFlags{} servicesCommand := &cobra.Command{ Use: "services", - Short: "Interact with devbox services", + Short: "Interact with devbox services.", + Long: "Interact with devbox services. Services start in a new shell. " + + "Plugin services use envrinment variables specified by plugin unless " + + "overridden by the user. To override plugin environment variables, use " + + "the --env or --env-file flag. You may also override in devbox.json by " + + "using the `env` field or exporting an environment variable in the " + + "init hook.", PersistentPreRunE: func(cmd *cobra.Command, args []string) error { preruns := append([]cobraFunc{ensureNixInstalled}, persistentPreRunE...) for _, fn := range preruns { From c7fa7bedf446f72c08ddbe6fd653a65ac87727f7 Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Wed, 18 Oct 2023 15:11:52 -0700 Subject: [PATCH 5/6] Fix test --- examples/stacks/lapp-stack/devbox.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/examples/stacks/lapp-stack/devbox.json b/examples/stacks/lapp-stack/devbox.json index 3f8e6a71483..6b5af23e64c 100644 --- a/examples/stacks/lapp-stack/devbox.json +++ b/examples/stacks/lapp-stack/devbox.json @@ -7,7 +7,8 @@ "apache@2.4" ], "env": { - "PGPORT": "5432" + "PGPORT": "5432", + "PGHOST": "/tmp/devbox/lapp" }, "shell": { "scripts": { @@ -19,7 +20,6 @@ "init_db": "initdb", "run_test": [ "mkdir -p /tmp/devbox/lapp", - "export PGHOST=/tmp/devbox/lapp", "initdb", "devbox services start", "echo 'sleep 1 second for the postgres server to initialize.' && sleep 1", From a01488c7258f0282777a5614bd79958ea7e6aceb Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Thu, 19 Oct 2023 11:50:03 -0700 Subject: [PATCH 6/6] Requested changes --- examples/stacks/lepp-stack/devbox.json | 1 - internal/boxcli/services.go | 2 +- internal/impl/envpath/pathlists.go | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/examples/stacks/lepp-stack/devbox.json b/examples/stacks/lepp-stack/devbox.json index 64dbe27fc2a..f0b7931a518 100644 --- a/examples/stacks/lepp-stack/devbox.json +++ b/examples/stacks/lepp-stack/devbox.json @@ -22,7 +22,6 @@ "init_db": "initdb", "run_test": [ "mkdir -p /tmp/devbox/lepp", - "export PGHOST=/tmp/devbox/lepp", "rm -rf .devbox/virtenv/postgresql/data", "initdb", "devbox services up -b", diff --git a/internal/boxcli/services.go b/internal/boxcli/services.go index 17fdebcb8b5..f2f79561846 100644 --- a/internal/boxcli/services.go +++ b/internal/boxcli/services.go @@ -50,7 +50,7 @@ func servicesCmd(persistentPreRunE ...cobraFunc) *cobra.Command { Use: "services", Short: "Interact with devbox services.", Long: "Interact with devbox services. Services start in a new shell. " + - "Plugin services use envrinment variables specified by plugin unless " + + "Plugin services use environment variables specified by plugin unless " + "overridden by the user. To override plugin environment variables, use " + "the --env or --env-file flag. You may also override in devbox.json by " + "using the `env` field or exporting an environment variable in the " + diff --git a/internal/impl/envpath/pathlists.go b/internal/impl/envpath/pathlists.go index 6554248c96a..625d8cd6c78 100644 --- a/internal/impl/envpath/pathlists.go +++ b/internal/impl/envpath/pathlists.go @@ -38,7 +38,7 @@ func JoinPathLists(pathLists ...string) string { } func RemoveFromPath(path, pathToRemove string) string { - paths := strings.Split(path, string(os.PathListSeparator)) + paths := filepath.SplitList(path) // Create a new slice to store the modified paths var newPaths []string