More cleanup following review; config is no longer relative to .envrc

Author: pstephan-geico-external

internal/boxcli/generate.go

@@ -6,7 +6,6 @@ package boxcli
 import (
 	"cmp"
 	"fmt"
-	"path/filepath"
 	"regexp"
 
 	"github.com/pkg/errors"
@@ -163,8 +162,7 @@ func direnvCmd() *cobra.Command {
 		&flags.envrcDir, "envrc-dir", "",
 		"path to directory where the .envrc file should be generated.\n"+
 			"If not specified, the .envrc file will be generated in the same directory as\n"+
-			"the devbox.json config file. Also, when specified along with --config, the config file\n"+
-			"location will be relative to the .envrc file location.")
+			"the devbox.json.")
 
 	flags.config.register(command)
 	return command
@@ -294,59 +292,21 @@ func runGenerateDirenvCmd(cmd *cobra.Command, flags *generateCmdFlags) error {
 			cmd.OutOrStdout(), devopt.EnvFlags(flags.envFlag), flags.config.path)
 	}
 
-	// Determine the directories for .envrc and config
-	configDir, envrcDir, err := determineDirenvDirs(flags.config.path, flags.envrcDir)
-	if err != nil {
-		return errors.WithStack(err)
-	}
-
-	generateOpts := devopt.EnvrcOpts{
-		EnvrcDir:  envrcDir,
-		ConfigDir: configDir,
-		EnvFlags:  devopt.EnvFlags(flags.envFlag),
-	}
-
 	box, err := devbox.Open(&devopt.Opts{
-		Dir:         filepath.Join(envrcDir, configDir),
+		Dir:         flags.config.path,
 		Environment: flags.config.environment,
 		Stderr:      cmd.ErrOrStderr(),
 	})
 	if err != nil {
 		return errors.WithStack(err)
 	}
 
-	return box.GenerateEnvrcFile(
-		cmd.Context(), flags.force, generateOpts)
-}
-
-// Returns canonical paths for configDir and envrcDir. Both locations are relative to the current
-// working directory when provided to this function. However, since the config file will ultimately
-// be relative to the .envrc file, we need to determine the relative path from envrcDir to configDir.
-func determineDirenvDirs(configDir, envrcDir string) (string, string, error) {
-	// If envrcDir is not specified, we will use the configDir as the location for .envrc. This is
-	// for backward compatibility (prior to the --envrc-dir flag being introduced).
-	if envrcDir == "" {
-		return "", configDir, nil
-	}
-
-	// If no configDir is specified, it will be assumed to be in the same directory as the .envrc file
-	// which means we can just return an empty configDir.
-	if configDir == "" {
-		return "", envrcDir, nil
-	}
-
-	relativeConfigDir, err := filepath.Rel(envrcDir, configDir)
-	if err != nil {
-		return "", "", errors.Wrapf(err, "failed to determine relative path from %s to %s", envrcDir, configDir)
-	}
-
-	// If the relative path is ".", it means configDir is the same as envrcDir. Leaving it as "."
-	// will result in the .envrc containing "--config .", which is fine, but unnecessary and also
-	// a change from the previous behavior. So we will return an empty string for relativeConfigDir
-	// which will result in the .envrc file not containing the "--config" flag at all.
-	if relativeConfigDir == "." {
-		relativeConfigDir = ""
+	generateEnvrcOpts := devopt.EnvrcOpts{
+		EnvFlags:  devopt.EnvFlags(flags.envFlag),
+		Force:     flags.force,
+		EnvrcDir:  flags.envrcDir,
+		ConfigDir: flags.config.path,
 	}
 
-	return relativeConfigDir, envrcDir, nil
+	return box.GenerateEnvrcFile(cmd.Context(), generateEnvrcOpts)
 }

internal/devbox/devbox.go

@@ -527,18 +527,25 @@ func (d *Devbox) GenerateDockerfile(ctx context.Context, generateOpts devopt.Gen
 	}))
 }
 
-func PrintEnvrcContent(w io.Writer, opts devopt.EnvFlags, configDir string) error {
-	return generate.EnvrcContent(w, opts, configDir)
+func PrintEnvrcContent(w io.Writer, envFlags devopt.EnvFlags, configDir string) error {
+	return generate.EnvrcContent(w, envFlags, configDir)
 }
 
 // GenerateEnvrcFile generates a .envrc file that makes direnv integration convenient
-func (d *Devbox) GenerateEnvrcFile(ctx context.Context, force bool, opts devopt.EnvrcOpts) error {
+func (d *Devbox) GenerateEnvrcFile(ctx context.Context, opts devopt.EnvrcOpts) error {
 	ctx, task := trace.NewTask(ctx, "devboxGenerateEnvrc")
 	defer task.End()
 
-	envrcfilePath := filepath.Join(opts.EnvrcDir, ".envrc")
-	filesExist := fileutil.Exists(envrcfilePath)
-	if !force && filesExist {
+	// If no envrcDir was specified, use the configDir. This is for backward compatibility
+	// where the .envrc was placed in the same location as specified by --config. Note that
+	// if that is also blank, the .envrc will be generated in the current working directory.
+	if opts.EnvrcDir == "" {
+		opts.EnvrcDir = opts.ConfigDir
+	}
+
+	envrcFilePath := filepath.Join(opts.EnvrcDir, ".envrc")
+	filesExist := fileutil.Exists(envrcFilePath)
+	if !opts.Force && filesExist {
 		return usererr.New(
 			"A .envrc is already present in %q. Remove it or use --force to overwrite it.",
 			opts.EnvrcDir,

internal/devbox/devopt/devboxopts.go

@@ -36,6 +36,7 @@ type EnvFlags struct {
 
 type EnvrcOpts struct {
 	EnvFlags
+	Force     bool
 	EnvrcDir  string
 	ConfigDir string
 }

internal/devbox/generate/devcontainer_util.go

@@ -161,15 +161,47 @@ func CreateEnvrc(ctx context.Context, opts devopt.EnvrcOpts) error {
 		flags = append(flags, fmt.Sprintf("--env-file %s", opts.EnvFile))
 	}
 
+	configDir, err := getRelativePathToConfig(opts.EnvrcDir, opts.ConfigDir)
+	if err != nil {
+		return err
+	}
+
 	t := template.Must(template.ParseFS(tmplFS, "tmpl/envrc.tmpl"))
 
 	// write content into file
 	return t.Execute(file, map[string]string{
 		"EnvFlag":   strings.Join(flags, " "),
-		"ConfigDir": formatConfigDirArg(opts.ConfigDir),
+		"ConfigDir": formatConfigDirArg(configDir),
 	})
 }
 
+// Returns the relative path from sourceDir to configDir, or an error if it cannot be determined.
+func getRelativePathToConfig(sourceDir string, configDir string) (string, error) {
+	absConfigDir, err := filepath.Abs(configDir)
+	if err != nil {
+		return "", fmt.Errorf("failed to get absolute path for config dir: %w", err)
+	}
+
+	absSourceDir, err := filepath.Abs(sourceDir)
+	if err != nil {
+		return "", fmt.Errorf("failed to get absolute path for source dir: %w", err)
+	}
+
+	// We don't want the path if the config dir is a parent of the envrc dir. This way
+	// the config will be found when it recursively searches for it through the parent tree.
+	if strings.HasPrefix(absSourceDir, absConfigDir) {
+		return "", nil
+	}
+
+	relPath, err := filepath.Rel(absSourceDir, absConfigDir)
+	if err != nil {
+		// If a relative path cannot be computed, return the absolute path of configDir
+		return absConfigDir, nil
+	}
+
+	return relPath, nil
+}
+
 func (g *Options) getDevcontainerContent() *devcontainerObject {
 	// object that gets written in devcontainer.json
 	devcontainerContent := &devcontainerObject{
@@ -221,8 +253,7 @@ func (g *Options) getDevcontainerContent() *devcontainerObject {
 }
 
 func EnvrcContent(w io.Writer, envFlags devopt.EnvFlags, configDir string) error {
-	tmplName := "envrcContent.tmpl"
-	t := template.Must(template.ParseFS(tmplFS, "tmpl/"+tmplName))
+	t := template.Must(template.ParseFS(tmplFS, "tmpl/envrcContent.tmpl"))
 	envFlag := ""
 	if len(envFlags.EnvMap) > 0 {
 		for k, v := range envFlags.EnvMap {
@@ -231,19 +262,16 @@ func EnvrcContent(w io.Writer, envFlags devopt.EnvFlags, configDir string) error
 	}
 
 	return t.Execute(w, map[string]string{
-		"JSONPath":  filepath.Join(configDir, "devbox.json"),
-		"LockPath":  filepath.Join(configDir, "devbox.lock"),
 		"EnvFlag":   envFlag,
 		"EnvFile":   envFlags.EnvFile,
 		"ConfigDir": formatConfigDirArg(configDir),
 	})
 }
 
 func formatConfigDirArg(configDir string) string {
-	configDirArg := ""
-	if configDir != "" {
-		configDirArg = "--config " + configDir
+	if configDir == "" {
+		return ""
 	}
 
-	return configDirArg
+	return "--config " + configDir
 }

internal/devbox/generate/tmpl/envrcContent.tmpl

@@ -1,6 +1,6 @@
 use_devbox() {
-    watch_file {{ .JSONPath }} {{ .LockPath }}
-    eval "$(devbox shellenv --init-hook --install --no-refresh-alias{{ if .EnvFlag}} {{ .EnvFlag }}{{ end }}{{ if .ConfigDir }} {{ .ConfigDir }}{{ end }})"
+    eval "$(devbox shellenv --init-hook --install --no-refresh-alias{{ if .EnvFlag }} {{ .EnvFlag }}{{ end }}{{ if .ConfigDir }} {{ .ConfigDir }}{{ end }})"
+    watch_file $DEVBOX_PROJECT_ROOT/devbox.json $DEVBOX_PROJECT_ROOT/devbox.lock
 }
 use devbox
 {{ if .EnvFile }}

testscripts/generate/direnv-config-envflag.test.txt

@@ -0,0 +1,26 @@
+# Testscript to validate generating the contents of the .envrc file.
+# Note that since --envrc-dir was NOT specified, the .envrc will be in the `dir` directory and
+# the config will be found there, which means the `--print-env` doesn't need to specify the dir.
+# This matches the mode of operation prior to the addition of the --envrc-dir flag.
+
+mkdir dir
+exec devbox init dir
+exists dir/devbox.json
+
+exec devbox generate direnv --env x=y --config dir
+grep 'eval "\$\(devbox generate direnv --print-envrc --env x=y\)"' dir/.envrc
+
+cd dir
+exec devbox generate direnv --print-envrc --env x=y
+
+cmp stdout ../expected-results.txt
+
+# Note: The contents of the following file ends with two blank lines. This is
+# necessary to match the blank line that follows "use devbox" in the actual output.
+-- expected-results.txt --
+use_devbox() {
+    eval "$(devbox shellenv --init-hook --install --no-refresh-alias --env x=y )"
+    watch_file $DEVBOX_PROJECT_ROOT/devbox.json $DEVBOX_PROJECT_ROOT/devbox.lock
+}
+use devbox
+

testscripts/generate/direnv-config.test.txt

@@ -0,0 +1,26 @@
+# Testscript to validate generating the contents of the .envrc file.
+# Note that since --envrc-dir was NOT specified, the .envrc will be in the `dir` directory and
+# the config will be found there, which means the `--print-env` doesn't need to specify the dir.
+# This matches the mode of operation prior to the addition of the --envrc-dir flag.
+
+mkdir dir
+exec devbox init dir
+exists dir/devbox.json
+
+exec devbox generate direnv --config dir
+grep 'eval "\$\(devbox generate direnv --print-envrc\)"' dir/.envrc
+
+cd dir
+exec devbox generate direnv --print-envrc
+
+cmp stdout ../expected-results.txt
+
+# Note: The contents of the following file ends with two blank lines. This is
+# necessary to match the blank line that follows "use devbox" in the actual output.
+-- expected-results.txt --
+use_devbox() {
+    eval "$(devbox shellenv --init-hook --install --no-refresh-alias)"
+    watch_file $DEVBOX_PROJECT_ROOT/devbox.json $DEVBOX_PROJECT_ROOT/devbox.lock
+}
+use devbox
+

testscripts/generate/direnv-envflag-config.test.txt

@@ -1,10 +1,21 @@
 # Testscript to validate generating the contents of the .envrc file.
-# Note that since --envrc-dir was NOT specified, the .envrc will be in the `dir` directory and
-# the config will be found there, which means the `--print-env` doesn't need to specify the dir.
-
-mkdir dir
-exec devbox init dir
-exec devbox generate direnv --env x=y --config dir
-exists dir/.envrc
-exists dir/devbox.json
-grep 'eval "\$\(devbox generate direnv --print-envrc --env x=y\)"' dir/.envrc
+
+exec devbox init
+exists devbox.json
+
+exec devbox generate direnv --env x=y
+grep 'eval "\$\(devbox generate direnv --print-envrc --env x=y\)"' .envrc
+
+exec devbox generate direnv --print-envrc --env x=y
+
+cmp stdout expected-results.txt
+
+# Note: The contents of the following file ends with two blank lines. This is
+# necessary to match the blank line that follows "use devbox" in the actual output.
+-- expected-results.txt --
+use_devbox() {
+    eval "$(devbox shellenv --init-hook --install --no-refresh-alias --env x=y )"
+    watch_file $DEVBOX_PROJECT_ROOT/devbox.json $DEVBOX_PROJECT_ROOT/devbox.lock
+}
+use devbox
+