-
Notifications
You must be signed in to change notification settings - Fork 252
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
base: main
Are you sure you want to change the base?
Add --envrc-dir flag to allow specifying location of direnv config #2629
Conversation
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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 bothEnvrcDir
andConfigDir
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 |
There was a problem hiding this comment.
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
.
// 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.
There was a problem hiding this 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
"path to directory where the .envrc file should be generated. "+ | ||
"If not specified, the .envrc file will be generated in "+ | ||
"the current working directory.") | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
// 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 | ||
} |
There was a problem hiding this comment.
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.
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 = "" | ||
} |
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
{{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 }})" |
There was a problem hiding this comment.
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.
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 thedevbox.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 commanddevbox generate direnv --envrc-dir ABC --config ABC/XYZ
would createABC/.envrc
and within that would be the commandeval "$(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.