Skip to content

Add --envrc-dir flag to allow specifying location of direnv config #2629

New issue

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

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

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

pstephan-geico-external

Summary

Fixes #2459 - devbox generate direnv does not respect --config

It seems that the previous mode of operation for devbox generate direnv --config <some-dir> used --config to determine the location of the resulting .envrc file, rather than as the location of the devbox.json file. But in some cases it is desired to be able to specify not only the location of not only the direnv .envrc file, but also the devbox config file. Or at least to be able to specify just the location of the devbox config with the .envrc being in the current directory.

To accomplish this, without breaking the current mode of operation, a new parameter is added, --envrc-dir, which specifies the location of the resulting .envrc file. For example, the command devbox generate direnv --envrc-dir ABC --config ABC/XYZ would create ABC/.envrc and within that would be the command eval "$(devbox generate direnv --print-envrc --config XYZ)"

Without the new --envrc-dir param, operation is the same as it was previously, even with the --config (which will be used to specify the location of the .envrc file, NOT the devbox config file, devbox.json.

How was it tested?

Several new tests have been created to cover cases with and without the new --envrc-dir param.

Community Contribution License

All community contributions in this pull request are licensed to the project
maintainers under the terms of the
Apache 2 License.

By creating this pull request, I represent that I have the right to license the
contributions to the project maintainers under the Apache 2 License as stated in
the
Community Contribution License.

Copy link

@darch-geico-external darch-geico-external left a comment

Choose a reason for hiding this comment

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

I think I only spotted one actual bug.

Copy link

@darch-geico-external darch-geico-external left a comment

Choose a reason for hiding this comment

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

Obviously still needs review from an actual maintainer, but I don't see any other issues from here.

Copy link

@jguluarte jguluarte left a comment

Choose a reason for hiding this comment

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

LGTM

@LucilleH LucilleH requested a review from Copilot July 1, 2025 19:12
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds a new --envrc-dir flag to the devbox generate direnv command so users can specify where the .envrc file is written separately from the devbox config.
Key changes:

  • Introduce EnvrcOpts to carry both EnvrcDir and ConfigDir through internal APIs
  • Update templates (envrc.tmpl, envrcContent.tmpl) to respect both flags
  • Extend CLI (boxcli/generate.go), backend logic (determineDirenvDirs), and tests to cover --envrc-dir behavior

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
testscripts/generate/*.test.txt Added and updated shells scripts to validate new --envrc-dir flag
internal/devbox/generate/tmpl/envrcContent.tmpl Updated template to prefix files and include --config when set
internal/devbox/generate/tmpl/envrc.tmpl Adjusted embedded eval invocation to emit --config if needed
internal/devbox/generate/devcontainer_util.go Refactored CreateEnvrc to take EnvrcOpts and write to EnvrcDir
internal/devbox/devopt/devboxopts.go Introduced EnvrcOpts struct embedding EnvFlags
internal/devbox/devbox.go Updated GenerateEnvrcFile, PrintEnvrcContent, and related flows
internal/boxcli/generate.go Registered --envrc-dir flag, wired into runGenerateDirenvCmd
Comments suppressed due to low confidence (2)

internal/devbox/generate/tmpl/envrcContent.tmpl:1

  • [nitpick] The template variable .Dir is ambiguous; consider renaming it to .ConfigDir so its purpose is clearer in both templates.
{{define "DirPrefix"}}{{ if .Dir }}{{ .Dir }}/{{ end }}{{end}}

internal/devbox/devbox.go:558

  • [nitpick] When opts.EnvrcDir is empty, this prints "", which may confuse users. Consider special-casing the message to say "current directory" instead of empty quotes.
	ux.Fsuccessf(d.stderr, "generated .envrc file in %q.\n", opts.EnvrcDir)

cmd.Context(), flags.force, generateOpts)
}

// Returns cononical paths for configDir and envrcDir. Both locations are relative to the current
Copy link
Preview

Copilot AI Jul 1, 2025

Choose a reason for hiding this comment

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

Fix the typo 'cononical' to 'canonical' in the comment for determineDirenvDirs.

Suggested change
// Returns cononical paths for configDir and envrcDir. Both locations are relative to the current
// Returns canonical paths for configDir and envrcDir. Both locations are relative to the current

Copilot uses AI. Check for mistakes.

@mikeland73 mikeland73 self-requested a review July 1, 2025 20:39
Copy link
Contributor

@mikeland73 mikeland73 left a comment

Choose a reason for hiding this comment

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

This is a cool feature!

Two main changes I think you should do:

  • --envrc-dir should not control/override --config. It should be independent.
  • Don't pre-compute relative path between the dirs. Keep existing path semantics and only compute the relative path when you need to actually pting out the .envrc

Comment on lines +164 to +167
"path to directory where the .envrc file should be generated. "+
"If not specified, the .envrc file will be generated in "+
"the current working directory.")

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be:

"If not specified, the .envrc file will be generated in "+
			"the same directory as the devbox.json")

Since --config will determine where the .envrc file is created, which may not match the working directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe also clarify that if this flag is used, then we use relative path for --config

Comment on lines +330 to +334
// 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
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I agree with the semantics of this. --envrc-dir should dictate where the .envrc is created, not which config is used. The default for all devbox commands is the use the devbox.json in the current directory and if it doesn't exist, recursively check parent directories. I think we should preserve that.

In practice that means that using --envrc-dir and not using configDir will use the "current' devbox project, but create the .envrc where specified.

Comment on lines +336 to +347
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 = ""
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You're computing the relative path, and then joining again at the call site. Instead of precomputing the relative path, I think it would be simpler to do that when the .envrc is generated.

}

box, err := devbox.Open(&devopt.Opts{
Dir: flags.config.path,
Dir: filepath.Join(envrcDir, configDir),
Copy link
Contributor

Choose a reason for hiding this comment

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

keep this as flags.config.path,


generateOpts := devopt.EnvrcOpts{
EnvrcDir: envrcDir,
ConfigDir: configDir,
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to supply ConfigDir since it is already supplied by devopt.Opts

Copy link
Contributor

Choose a reason for hiding this comment

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

This is also a relative path (but named ConfigDir) which I'm afraid would get really confusing later on.

}

// Determine the directories for .envrc and config
configDir, envrcDir, err := determineDirenvDirs(flags.config.path, flags.envrcDir)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think determineDirenvDirs can be removed and just compute the relative path internally when generating the .envrc

}

t := template.Must(template.ParseFS(tmplFS, "tmpl/envrc.tmpl"))

// write content into file
return t.Execute(file, map[string]string{
"Flags": strings.Join(flags, " "),
"Dir": opts.ConfigDir,
Copy link
Contributor

Choose a reason for hiding this comment

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

Name this ConfigDir to reduce ambiguity.

Compute relative path here instead of generate.go

Copy link
Contributor

Choose a reason for hiding this comment

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

Make empty if envrcDir is not specified.

Comment on lines +1 to +4
{{define "DirPrefix"}}{{ if .Dir }}{{ .Dir }}/{{ end }}{{end}}
use_devbox() {
watch_file devbox.json devbox.lock
eval "$(devbox shellenv --init-hook --install --no-refresh-alias{{ if .EnvFlag }} {{ .EnvFlag }}{{ end }})"
watch_file {{template "DirPrefix" .}}devbox.json {{template "DirPrefix" .}}devbox.lock
eval "$(devbox shellenv --init-hook --install --no-refresh-alias {{ if .EnvFlag }}{{ .EnvFlag }}{{ end }} {{- if .Dir }}--config {{ .Dir -}}{{ end }})"
Copy link
Contributor

Choose a reason for hiding this comment

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

Templates are pretty hard to read. Ideally this is computed in Go code and just passed in. e.g.

watch_file {{ .RelativeDevboxJSONPath }} {{ .RelativeDevboxLockPath }}

You could do similar with --config flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

devbox generate direnv does not respect --config
4 participants