Skip to content

[shell] enable devbox add/rm to work for shells started in child directories #206

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

[shell] enable devbox add/rm to work for shells started in child directories #206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f0c6b6
[shell] enable devbox add/rm to work for shells started in child dire…
savil Oct 5, 2022
5dfc1cd
updates after rebasing over --config flag PR
savil Oct 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion boxcli/add.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@savil savil Oct 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

if envdir := os.Getenv(shellConfigEnvVar); envdir != "" {
dir = envdir
}
}
box, err := devbox.Open(dir, os.Stdout)
if err != nil {
return errors.WithStack(err)
}
Expand Down
3 changes: 3 additions & 0 deletions boxcli/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ import (
"github.com/spf13/cobra"
)

// Keep this env-var name same as its usage in shell.nix.tmpl
const shellConfigEnvVar = "DEVBOX_SHELL_CONFIG"

// to be composed into xyzCmdFlags structs
type configFlags struct {
path string
Expand Down
10 changes: 9 additions & 1 deletion boxcli/rm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,15 @@ func RemoveCmd() *cobra.Command {
}

func runRemoveCmd(_ *cobra.Command, args []string, flags *removeCmdFlags) error {
box, err := devbox.Open(flags.config.path, os.Stdout)

dir := flags.config.path
if dir == "" && devbox.IsDevboxShellEnabled() {
if envdir := os.Getenv(shellConfigEnvVar); envdir != "" {
dir = envdir
}
}

box, err := devbox.Open(dir, os.Stdout)
if err != nil {
return errors.WithStack(err)
}
Expand Down
20 changes: 12 additions & 8 deletions generate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func generate(rootPath string, plan *plansdk.Plan, files []string) error {
outPath := filepath.Join(rootPath, ".devbox/gen")

for _, file := range files {
err := writeFromTemplate(outPath, plan, file)
err := writeFromTemplate(outPath, plan, file, rootPath)
if err != nil {
return errors.WithStack(err)
}
Expand All @@ -36,7 +36,7 @@ func generate(rootPath string, plan *plansdk.Plan, files []string) error {
// Gitignore file is added to the .devbox directory
// TODO savil. Remove this hardcode from here, so this function can be generically defined again
// by accepting the files list parameter.
err := writeFromTemplate(filepath.Join(rootPath, ".devbox"), plan, ".gitignore")
err := writeFromTemplate(filepath.Join(rootPath, ".devbox"), plan, ".gitignore", rootPath)
if err != nil {
return errors.WithStack(err)
}
Expand All @@ -51,7 +51,7 @@ func generate(rootPath string, plan *plansdk.Plan, files []string) error {
return nil
}

func writeFromTemplate(path string, plan *plansdk.Plan, tmplName string) error {
func writeFromTemplate(path string, plan *plansdk.Plan, tmplName string, rootPath string) error {
embeddedPath := fmt.Sprintf("tmpl/%s.tmpl", tmplName)

// Should we clear the directory so we start "fresh"?
Expand All @@ -69,7 +69,7 @@ func writeFromTemplate(path string, plan *plansdk.Plan, tmplName string) error {
if err != nil {
return errors.WithStack(err)
}
t := template.Must(template.New(tmplName+".tmpl").Funcs(templateFuncs).ParseFS(tmplFS, embeddedPath))
t := template.Must(template.New(tmplName+".tmpl").Funcs(templateFuncs(rootPath)).ParseFS(tmplFS, embeddedPath))
return errors.WithStack(t.Execute(f, plan))
}

Expand All @@ -78,8 +78,12 @@ func toJSON(a any) string {
return string(data)
}

var templateFuncs = template.FuncMap{
"json": toJSON,
"contains": strings.Contains,
"debug": debug.IsEnabled,
func templateFuncs(rootPath string) template.FuncMap {

return template.FuncMap{
"json": toJSON,
"contains": strings.Contains,
"debug": debug.IsEnabled,
"configDir": func() string { return rootPath },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we pass this in as part of the template data instead of a function?

}
}
2 changes: 2 additions & 0 deletions tmpl/shell.nix.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be quoted to handle spaces.


# Undo the effects of `nix-shell --pure` on SSL certs.
# See https://github.com/NixOS/nixpkgs/blob/dae204faa0243b4d0c0234a5f5f83a2549ecb5b7/pkgs/stdenv/generic/setup.sh#L677-L685
Expand Down