[shell] enable devbox add/rm to work for shells started in child directories #206
Conversation
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
|
Converted to Draft. Will send another related PR based on discussion with @loreto and then circle back to this one. |
savil
force-pushed
the
savil/add-rm-from-child-folder
branch
from
1caa0de to
3f0c6b6
Compare
|
um, still working on this... |
|
We had some discussion about a generic way of overriding flags with env-vars but I'd prefer keeping this PR scoped down. We can always do that separately. |
| @@ -32,7 +32,13 @@ func AddCmd() *cobra.Command { | |||
|
|
|||
| func addCmdFunc(flags *addCmdFlags) runFunc { | |||
| return func(cmd *cobra.Command, args []string) error { | |||
| box, err := devbox.Open(flags.config.path, os.Stdout) | |||
| dir := flags.config.path | |||
| if dir == "" && devbox.IsDevboxShellEnabled() { | |||
There was a problem hiding this comment.
I think I missed some of the discussion around this feature, so apologies if I'm rehashing some stuff.
I'm trying to think through what the expected behavior would be here.
- If I'm in a devbox shell and cd into another devbox directory, is it obvious that add/rm will be operating on the shell's config and not the current directory's config?
- Similarly, it's weird that add and remove will affect a different config than all of the other commands. Or are we going to leave all the other commands disabled?
- What happens if my devbox directory is renamed or moved while I'm in a shell?
- Does this feature provide a big enough benefit now that we search parent directories for the config?
I want to say that if we're adding this feature, then we should disallow setting the config flag when in a shell and print a warning when the env var is overriding another devbox.json. Also, if the path in the env var becomes invalid, then we should tell the user to restart their shell. Thoughts?
There was a problem hiding this comment.
oh ho 🤦🏾 I was on auto-pilot and rebased this PR onto the --config flag parent PR, without thinking it through.
With the new --config flag functionality, this PR is not needed! yay!
You are totally right. Abandoning.
| "json": toJSON, | ||
| "contains": strings.Contains, | ||
| "debug": debug.IsEnabled, | ||
| "configDir": func() string { return rootPath }, |
There was a problem hiding this comment.
Can we pass this in as part of the template data instead of a function?
| @@ -26,6 +26,8 @@ mkShell { | |||
| # exec a devbox shell. | |||
| export IN_NIX_SHELL=0 | |||
| export DEVBOX_SHELL_ENABLED=1 | |||
| # Keep this env-var name same as boxcli.shellConfigEnvVar | |||
| export DEVBOX_SHELL_CONFIG={{ configDir }} | |||
There was a problem hiding this comment.
This needs to be quoted to handle spaces.
## Summary **Motivation** We want to introduce a `--config` flag and deprecate the use of config as an argument. 1. **ephemeral shells**. By enabling a way to (eventually) do `devbox shell --config path/to/devbox/json nodejs-18_x`, or even more simply `devbox shell nodejs-18_x`. 2. **using devbox add/rm from shells started in parent directories**: today, if I have `my_sub_directory/devbox.json` and start `devbox shell my_sub_directory/devbox.json` from the root of the repository, then doing `devbox add` or `devbox rm` inside this shell will fail. - A first attempt at fixing this was in #206. This PR (#225) is a generalization of that idea. I'll be stacking #206 on top of this PR, and enabling the `--config` flag to override the env-var. <insert motivation> ## How was it tested? **preliminary steps** - i added `nodejs-18_x` to `testdata/nodejs/nodejs-18` - I (temporarily) deleted the `devbox.json` at the root of this repo. This is to ensure I verify that the CLI still complains if no devbox.json is found. ``` # giving a path > cd testdata/nodejs > devbox shell nodejs-18 Warning: devbox shell <path> is deprecated, use devbox shell --config <path> instead Installing nix packages. This may take a while...done. Starting a devbox shell... > which node # get nix store path > exit # using --config > devbox shell -c nodejs-18 > which node # get nix store path > exit # using neither path nor --config > cd testdata/nodejs/node-18 > devbox shell > which node # get nix path > exit # using neither path nor --config, where devbox.json resides in a parent directory > cd testdata/nodejs/nodejs-18/fake-dir > devbox shell which node # get nix path > exit # using both path and --config > cd testdata/nodejs > devbox shell -c nodejs-18 nodejs-18 Error: Cannot specify devbox.json's path via both --config and the command arguments. Please use --config only. # No devbox.json in parent dirs > cd testdata/nodejs > devbox shell Error: No devbox.json found in this directory, or any parent directories. Did you run `devbox init` yet? # giving a path where no devbox.json exists > devbox shell nodejs-18/fake-dir Warning: devbox shell <path> is deprecated, use devbox shell --config <path> instead Error: No devbox.json found in nodejs-18/fake-dir. Did you run `devbox init` yet? ```
Summary
When doing
devbox shell <dir>, we wantdevbox addandrmto continue working.This PR enables this by setting an environment variable when within devbox shell that
contains the directory of devbox.json (config).
In
devbox addanddevbox rmwe check if this environment-variable is set and use that as the starting point for the config-directory search.How was it tested?