Skip to content

Commit da26558

Browse files
mikeland73mikeland73
authored
[services] Ensure services have env vars + cleanup (#454)
## Summary Ensures services have correct environment when called in shell. This fixes a particular case where a plugin is installed and then a service is started without restarting the shell. Cleanup: * Removed feature flag * Simplified code by having plugin.Env return a slice instead of a map * Factored out service start/stop to make linter happy ## How was it tested? * `devbox shell` * `devbox add apacheHttpd && devbox service start apache`
1 parent b791d43 commit da26558

File tree

8 files changed

+88
-106
lines changed

8 files changed

+88
-106
lines changed

‎internal/boxcli/featureflag/pkgconfig.go‎

Lines changed: 0 additions & 3 deletions
This file was deleted.

‎internal/boxcli/info.go‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"github.com/pkg/errors"
88
"github.com/spf13/cobra"
99
"go.jetpack.io/devbox"
10-
"go.jetpack.io/devbox/internal/boxcli/featureflag"
1110
)
1211

1312
type infoCmdFlags struct {
@@ -19,7 +18,6 @@ func InfoCmd() *cobra.Command {
1918
flags := infoCmdFlags{}
2019
command := &cobra.Command{
2120
Use: "info <pkg>",
22-
Hidden: !featureflag.PKGConfig.Enabled(),
2321
Short: "Display package info",
2422
Args: cobra.ExactArgs(1),
2523
PreRunE: ensureNixInstalled,

‎internal/boxcli/services.go‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"github.com/pkg/errors"
88
"github.com/spf13/cobra"
99
"go.jetpack.io/devbox"
10-
"go.jetpack.io/devbox/internal/boxcli/featureflag"
1110
)
1211

1312
type servicesCmdFlags struct {
@@ -17,9 +16,8 @@ type servicesCmdFlags struct {
1716
func ServicesCmd() *cobra.Command {
1817
flags := servicesCmdFlags{}
1918
servicesCommand := &cobra.Command{
20-
Use: "services",
21-
Hidden: !featureflag.PKGConfig.Enabled(),
22-
Short: "Interact with devbox services",
19+
Use: "services",
20+
Short: "Interact with devbox services",
2321
}
2422

2523
lsCommand := &cobra.Command{

‎internal/impl/devbox.go‎

Lines changed: 33 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -130,18 +130,18 @@ func (d *Devbox) Add(pkgs ...string) error {
130130
if err := d.ensurePackagesAreInstalled(install); err != nil {
131131
return err
132132
}
133-
if featureflag.PKGConfig.Enabled() {
134-
for _, pkg := range pkgs {
135-
if err := plugin.PrintReadme(
136-
pkg,
137-
d.projectDir,
138-
d.writer,
139-
false, /*markdown*/
140-
); err != nil {
141-
return err
142-
}
133+
134+
for _, pkg := range pkgs {
135+
if err := plugin.PrintReadme(
136+
pkg,
137+
d.projectDir,
138+
d.writer,
139+
false, /*markdown*/
140+
); err != nil {
141+
return err
143142
}
144143
}
144+
145145
return d.printPackageUpdateMessage(install, pkgs)
146146
}
147147

@@ -165,10 +165,8 @@ func (d *Devbox) Remove(pkgs ...string) error {
165165
return err
166166
}
167167

168-
if featureflag.PKGConfig.Enabled() {
169-
if err := plugin.Remove(d.projectDir, uninstalledPackages); err != nil {
170-
return err
171-
}
168+
if err := plugin.Remove(d.projectDir, uninstalledPackages); err != nil {
169+
return err
172170
}
173171

174172
if err := d.ensurePackagesAreInstalled(uninstall); err != nil {
@@ -217,23 +215,18 @@ func (d *Devbox) Shell() error {
217215
return err
218216
}
219217

218+
env, err := plugin.Env(d.cfg.Packages, d.projectDir)
219+
if err != nil {
220+
return err
221+
}
222+
220223
opts := []nix.ShellOption{
221224
nix.WithPluginInitHook(strings.Join(pluginHooks, "\n")),
222225
nix.WithProfile(profileDir),
223226
nix.WithHistoryFile(filepath.Join(d.projectDir, shellHistoryFile)),
224227
nix.WithProjectDir(d.projectDir),
225-
}
226-
// TODO: separate package suggestions from shell planners
227-
if featureflag.PKGConfig.Enabled() {
228-
env, err := plugin.Env(d.cfg.Packages, d.projectDir)
229-
if err != nil {
230-
return err
231-
}
232-
opts = append(
233-
opts,
234-
nix.WithEnvVariables(env),
235-
nix.WithPKGConfigDir(filepath.Join(d.projectDir, plugin.VirtenvBinPath)),
236-
)
228+
nix.WithEnvVariables(env),
229+
nix.WithPKGConfigDir(filepath.Join(d.projectDir, plugin.VirtenvBinPath)),
237230
}
238231

239232
shell, err := nix.DetectShell(opts...)
@@ -295,24 +288,19 @@ func (d *Devbox) RunScript(scriptName string) error {
295288
return err
296289
}
297290

291+
env, err := plugin.Env(d.cfg.Packages, d.projectDir)
292+
if err != nil {
293+
return err
294+
}
295+
298296
opts := []nix.ShellOption{
299297
nix.WithPluginInitHook(strings.Join(pluginHooks, "\n")),
300298
nix.WithProfile(profileDir),
301299
nix.WithHistoryFile(filepath.Join(d.projectDir, shellHistoryFile)),
302300
nix.WithUserScript(scriptName, script.String()),
303301
nix.WithProjectDir(d.projectDir),
304-
}
305-
306-
if featureflag.PKGConfig.Enabled() {
307-
env, err := plugin.Env(d.cfg.Packages, d.projectDir)
308-
if err != nil {
309-
return err
310-
}
311-
opts = append(
312-
opts,
313-
nix.WithEnvVariables(env),
314-
nix.WithPKGConfigDir(filepath.Join(d.projectDir, plugin.VirtenvBinPath)),
315-
)
302+
nix.WithEnvVariables(env),
303+
nix.WithPKGConfigDir(filepath.Join(d.projectDir, plugin.VirtenvBinPath)),
316304
}
317305

318306
shell, err := nix.DetectShell(opts...)
@@ -346,18 +334,13 @@ func (d *Devbox) Exec(cmds ...string) error {
346334
return err
347335
}
348336

349-
env := []string{}
350-
virtenvBinPath := ""
351-
if featureflag.PKGConfig.Enabled() {
352-
envMap, err := plugin.Env(d.cfg.Packages, d.projectDir)
353-
if err != nil {
354-
return err
355-
}
356-
for k, v := range envMap {
357-
env = append(env, fmt.Sprintf("%s=%s", k, v))
358-
}
359-
virtenvBinPath = filepath.Join(d.projectDir, plugin.VirtenvBinPath) + ":"
337+
env, err := plugin.Env(d.cfg.Packages, d.projectDir)
338+
if err != nil {
339+
return err
360340
}
341+
342+
virtenvBinPath := filepath.Join(d.projectDir, plugin.VirtenvBinPath) + ":"
343+
361344
pathWithProfileBin := fmt.Sprintf("PATH=%s%s:$PATH", virtenvBinPath, profileBinDir)
362345
cmds = append([]string{pathWithProfileBin}, cmds...)
363346

@@ -572,13 +555,7 @@ func (d *Devbox) ensurePackagesAreInstalled(mode installMode) error {
572555
}
573556
fmt.Fprintln(d.writer, "done.")
574557

575-
if featureflag.PKGConfig.Enabled() {
576-
if err := plugin.RemoveInvalidSymlinks(d.projectDir); err != nil {
577-
return err
578-
}
579-
}
580-
581-
return nil
558+
return plugin.RemoveInvalidSymlinks(d.projectDir)
582559
}
583560

584561
func (d *Devbox) printPackageUpdateMessage(

‎internal/impl/generate.go‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,9 @@ func generateForShell(rootPath string, plan *plansdk.ShellPlan, pluginManager *p
5555
}
5656
}
5757

58-
if featureflag.PKGConfig.Enabled() {
59-
for _, pkg := range plan.DevPackages {
60-
if err := pluginManager.CreateFilesAndShowReadme(pkg, rootPath); err != nil {
61-
return err
62-
}
58+
for _, pkg := range plan.DevPackages {
59+
if err := pluginManager.CreateFilesAndShowReadme(pkg, rootPath); err != nil {
60+
return err
6361
}
6462
}
6563

‎internal/nix/shell.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -125,11 +125,11 @@ func WithHistoryFile(historyFile string) ShellOption {
125125
}
126126
}
127127

128-
func WithEnvVariables(envVariables map[string]string) ShellOption {
128+
// TODO: Consider removing this once plugins add env vars directly to binaries
129+
// via wrapper scripts.
130+
func WithEnvVariables(envVariables []string) ShellOption {
129131
return func(s *Shell) {
130-
for k, v := range envVariables {
131-
s.env = append(s.env, fmt.Sprintf("%s=%s", k, v))
132-
}
132+
s.env = append(s.env, envVariables...)
133133
}
134134
}
135135

‎internal/plugin/pkgcfg.go‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -103,8 +103,11 @@ func (m *Manager) CreateFilesAndShowReadme(pkg, projectDir string) error {
103103

104104
}
105105

106-
func Env(pkgs []string, projectDir string) (map[string]string, error) {
107-
env := map[string]string{}
106+
// Env returns the environment variables for the given plugins.
107+
// TODO: We should associate the env variables with the individual plugin
108+
// binaries via wrappers instead of adding to the environment everywhere.
109+
func Env(pkgs []string, projectDir string) ([]string, error) {
110+
env := []string{}
108111
for _, pkg := range pkgs {
109112
cfg, err := getConfigIfAny(pkg, projectDir)
110113
if err != nil {
@@ -114,7 +117,7 @@ func Env(pkgs []string, projectDir string) (map[string]string, error) {
114117
continue
115118
}
116119
for k, v := range cfg.Env {
117-
env[k] = v
120+
env = append(env, fmt.Sprintf("%s=%s", k, v))
118121
}
119122
}
120123
return env, nil
@@ -134,12 +137,13 @@ func createEnvFile(pkg, projectDir string) error {
134137
return err
135138
}
136139
env := ""
137-
for k, v := range envVars {
138-
escaped, err := json.Marshal(v)
140+
for _, val := range envVars {
141+
parts := strings.SplitN(val, "=", 2)
142+
escaped, err := json.Marshal(parts[1])
139143
if err != nil {
140144
return errors.WithStack(err)
141145
}
142-
env += fmt.Sprintf("export %s=%s\n", k, escaped)
146+
env += fmt.Sprintf("export %s=%s\n", parts[0], escaped)
143147
}
144148
filePath := filepath.Join(projectDir, VirtenvPath, pkg, "/env")
145149
if err = createDir(filepath.Dir(filePath)); err != nil {

‎internal/plugin/services.go‎

Lines changed: 36 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,10 @@ import (
44
"encoding/json"
55
"fmt"
66
"io"
7+
"os"
78
"os/exec"
89

10+
"github.com/samber/lo"
911
"go.jetpack.io/devbox/internal/boxcli/usererr"
1012
)
1113

@@ -49,50 +51,58 @@ func (s *Services) UnmarshalJSON(b []byte) error {
4951
}
5052

5153
func StartServices(pkgs, serviceNames []string, root string, w io.Writer) error {
52-
services, err := GetServices(pkgs, root)
53-
if err != nil {
54-
return err
55-
}
56-
for _, name := range serviceNames {
57-
service, found := services[name]
58-
if !found {
59-
return usererr.New("Service not found")
60-
}
61-
cmd := exec.Command("sh", "-c", service.Start)
62-
cmd.Stdout = w
63-
cmd.Stderr = w
64-
if err = cmd.Run(); err != nil {
65-
if len(serviceNames) == 1 {
66-
return usererr.WithUserMessage(err, "Service %q failed to start", name)
67-
}
68-
fmt.Fprintf(w, "Service %q failed to start. Error = %s\n", name, err)
69-
} else {
70-
fmt.Fprintf(w, "Service %q started\n", name)
71-
}
72-
}
73-
return nil
54+
return toggleServices(pkgs, serviceNames, root, w, startService)
7455
}
7556

7657
func StopServices(pkgs, serviceNames []string, root string, w io.Writer) error {
58+
return toggleServices(pkgs, serviceNames, root, w, stopService)
59+
}
60+
61+
type serviceAction int
62+
63+
const (
64+
startService serviceAction = iota
65+
stopService
66+
)
67+
68+
func toggleServices(
69+
pkgs,
70+
serviceNames []string,
71+
root string,
72+
w io.Writer,
73+
action serviceAction,
74+
) error {
7775
services, err := GetServices(pkgs, root)
7876
if err != nil {
7977
return err
8078
}
79+
envVars, err := Env(pkgs, root)
80+
if err != nil {
81+
return err
82+
}
8183
for _, name := range serviceNames {
8284
service, found := services[name]
8385
if !found {
8486
return usererr.New("Service not found")
8587
}
86-
cmd := exec.Command("sh", "-c", service.Stop)
88+
cmd := exec.Command(
89+
"sh",
90+
"-c",
91+
lo.Ternary(action == startService, service.Start, service.Stop),
92+
)
8793
cmd.Stdout = w
8894
cmd.Stderr = w
95+
cmd.Env = envVars
96+
cmd.Env = append(cmd.Env, os.Environ()...)
8997
if err = cmd.Run(); err != nil {
98+
actionString := lo.Ternary(action == startService, "start", "stop")
9099
if len(serviceNames) == 1 {
91-
return usererr.WithUserMessage(err, "Service %q failed to stop", name)
100+
return usererr.WithUserMessage(err, "Service %q failed to %s", name, actionString)
92101
}
93-
fmt.Fprintf(w, "Service %q failed to stop. Error = %s\n", name, err)
102+
fmt.Fprintf(w, "Service %q failed to %s. Error = %s\n", name, actionString, err)
94103
} else {
95-
fmt.Fprintf(w, "Service %q stopped\n", name)
104+
actionStringPast := lo.Ternary(action == startService, "started", "stopped")
105+
fmt.Fprintf(w, "Service %q %s\n", name, actionStringPast)
96106
}
97107
}
98108
return nil

0 commit comments

Comments
 (0)