[CLI] add --config flag #225

savil · 2022-10-07T19:27:03Z

Summary

Motivation
We want to introduce a --config flag and deprecate the use of config as an argument.

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.
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 [shell] enable devbox add/rm to work for shells started in child directories #206. This PR ([CLI] add --config flag #225) is a generalization of that idea. I'll be stacking [shell] enable devbox add/rm to work for shells started in child directories #206 on top of this PR, and enabling the --config flag to override the env-var.

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?

savil · 2022-10-07T19:27:16Z

Current dependencies on/for this PR:

main
- PR [CLI] add --config flag #225 👈
  - PR [shell] enable devbox add/rm to work for shells started in child directories #206

This comment was auto-generated by Graphite.

savil · 2022-10-07T19:47:51Z

I'll fix the linter errors (minor), but this is ready for review.

loreto · 2022-10-08T06:06:25Z

boxcli/build.go

 func BuildCmd() *cobra.Command {
-	flags := &docker.BuildFlags{}
+	flags := newBuildFlags()

 	command := &cobra.Command{
 		Use:   "build [<dir>]",


Update Use to not have [<dir>] – same for any other commands where this is changing

ah, yeah, I overlooked that

boxcli/generate.go

loreto · 2022-10-08T06:08:49Z

boxcli/init.go

@@ -9,19 +9,29 @@ import (
 	"go.jetpack.io/devbox"
 )

+type initCmdFlags struct {


So ... init is the one command I was still imagining would work as devbox init my/project instead of devbox init -c my/project

Thoughts on whether we should keep init working as before?

I don't feel strongly, I can change it back ....

How do you feel about keeping it for consistency?

loreto · 2022-10-08T06:17:44Z

boxcli/args.go

 	"github.com/pkg/errors"
 )

 // Functions that help parse arguments

 // If args empty, defaults to the current directory
 // Otherwise grabs the path from the first argument
-func pathArg(args []string) string {
+func pathArg(args []string, flags *configFlags) string {


1

If you want to keep the logic simpler, I think it'd be ok to error if both flag and path as an argument are specified at the same time:

path-as-arg: warning

flag: no warning

both: error

2

I think you somehow need to distinguish between the flag being explicitly set and not set. At least I was assuming that:

devbox add foo looks for devbox.json in a parent directory if it's not in the current directory.

devbox add foo -c /my/project looks for a devbox.json in /my/project only (and not in any of it's parent directories). By extension that means devbox add foo -c "." only looks in the current directory and not in the parents.

devbox add foo -c /my/project/my-file.json treats my-file.json as the config file.

That said, open to a different approach. How were you thinking it should work?

I think it'd be ok to error if both flag and path as an argument are specified at the same time:

👍🏾

That said, open to a different approach. How were you thinking it should work?

I'd been assuming we'd match the current behavior i.e. devbox add foo -c /my/project will treat /my/project as the starting point of the search and traverse up to the parent directories.

Happy to change that logic to your proposal. It does feel more clear to not do the magic search if the user is specifying the config-path directly.

How do you feel about doing that as a followup PR? I fear a bigger refactoring is needed because the search-parent-dirs logic is inside devbox.go. If its simple, I'll fold the change into this PR.

If its simple, I'll fold the change into this PR.

narrator voice it was simple, and folded into this PR.

loreto · 2022-10-08T06:19:22Z

boxcli/config.go

+
+func registerConfigFlags(cmd *cobra.Command, flags *configFlags) {
+	cmd.Flags().StringVarP(
+		&flags.path, "config", "c", currentDir, "path to directory containing a devbox.json config file",


If you need to distinguish between the flag being set and not set, you might want the default value to actually be the empty string.

cobra has an api something like if cmd.Flags().Changed(flagName) {

I suppose it depends on where I need to distinguish this.

loreto · 2022-10-08T06:29:02Z

Two more thoughts:

The warning message feels too long – it should be more succinct. Something like WARNING: devbox shell <path> is deprecated, use devbox shell --config <path> instead
While I'm happy enough with --config, I'm not sure I love -c when seeing it in the examples. I'm not entirely sure people will be able to immediately guess that in devbox shell -c nodejs-18 the -c stands for config. Thoughts?

savil

@loreto Thanks for the review! I'll fix up early tomorrow.

Two more thoughts:
The warning message feels too long – it should be more succinct. Something like WARNING: devbox shell is deprecated, use devbox shell --config instead

sounds good, easy change.

While I'm happy enough with --config, I'm not sure I love -c when seeing it in the examples. I'm not entirely sure people will be able to immediately guess that in devbox shell -c nodejs-18 the -c stands for config. Thoughts?

Not sure I follow the concern here. We do still have the --config flag. The -c is just a shorthand for that flag for users who get familiar with devbox and don't wanna type as much. Could you elaborate a bit more?

savil · 2022-10-10T15:44:32Z

boxcli/args.go

 	"github.com/pkg/errors"
 )

 // Functions that help parse arguments

 // If args empty, defaults to the current directory
 // Otherwise grabs the path from the first argument
-func pathArg(args []string) string {
+func pathArg(args []string, flags *configFlags) string {


I think it'd be ok to error if both flag and path as an argument are specified at the same time:

👍🏾

That said, open to a different approach. How were you thinking it should work?

I'd been assuming we'd match the current behavior i.e. devbox add foo -c /my/project will treat /my/project as the starting point of the search and traverse up to the parent directories.

Happy to change that logic to your proposal. It does feel more clear to not do the magic search if the user is specifying the config-path directly.

How do you feel about doing that as a followup PR? I fear a bigger refactoring is needed because the search-parent-dirs logic is inside devbox.go. If its simple, I'll fold the change into this PR.

savil · 2022-10-10T15:45:19Z

boxcli/build.go

 func BuildCmd() *cobra.Command {
-	flags := &docker.BuildFlags{}
+	flags := newBuildFlags()

 	command := &cobra.Command{
 		Use:   "build [<dir>]",


ah, yeah, I overlooked that

savil · 2022-10-10T15:46:15Z

boxcli/config.go

+
+func registerConfigFlags(cmd *cobra.Command, flags *configFlags) {
+	cmd.Flags().StringVarP(
+		&flags.path, "config", "c", currentDir, "path to directory containing a devbox.json config file",


cobra has an api something like if cmd.Flags().Changed(flagName) {

I suppose it depends on where I need to distinguish this.

savil · 2022-10-10T15:47:18Z

boxcli/init.go

@@ -9,19 +9,29 @@ import (
 	"go.jetpack.io/devbox"
 )

+type initCmdFlags struct {


I don't feel strongly, I can change it back ....

savil · 2022-10-12T16:48:24Z

@gcurtis @mikeland86 adding y'all as reviewers since others may be away for a bit.

gcurtis

LGTM! Can we just add some tests for the config searching behavior?

Another minor, optional change is to pass around the flag structs as values if they're not being modified.

gcurtis · 2022-10-12T17:07:29Z

boxcli/rm.go

 	return command
 }

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


Can we just remove the *cobra.Command parameter entirely?

boxcli/shell.go

gcurtis · 2022-10-12T17:29:17Z

boxcli/config.go

+func registerConfigFlags(cmd *cobra.Command, flags *configFlags) {
+	cmd.Flags().StringVarP(
+		&flags.path, "config", "c", "", "path to directory containing a devbox.json config file",
+	)
+}


(Optional) making this a method instead of a package function might make it easier to find.

Suggested change

func registerConfigFlags(cmd *cobra.Command, flags *configFlags) {

cmd.Flags().StringVarP(

&flags.path, "config", "c", "", "path to directory containing a devbox.json config file",

)

}

func (c *configFlags) register(cmd *cobra.Command) {

cmd.Flags().StringVarP(

&c.path, "config", "c", "", "path to directory containing a devbox.json config file",

)

}

boxcli/add.go

boxcli/build.go

gcurtis · 2022-10-12T17:35:41Z

boxcli/build.go

+func newBuildFlags() *buildCmdFlags {
+	return &buildCmdFlags{
+		docker: &docker.BuildFlags{},
+	}
+}


If buildCmdFlags is used as a value then this can go away.

Suggested change

func newBuildFlags() *buildCmdFlags {

return &buildCmdFlags{

docker: &docker.BuildFlags{},

}

}

gcurtis · 2022-10-12T17:39:42Z

devbox.go

 }

+// findConfigDir


gcurtis · 2022-10-12T17:45:29Z

devbox.go

+		switch mode := fi.Mode(); {
+		case mode.IsDir():
+			if !plansdk.FileExists(filepath.Join(absDir, configFilename)) {
+				return "", missingDevboxJSONError(dir, false /*didCheckParents*/)


This is a personal style thing, but sometimes I find that using a struct directly can help keep things readable. For example, if you have:

type missingConfigError struct { dir string checkedParents bool } // Implement error

then this line becomes:

return "", &missingConfigError{dir: dir, checkedParents: false}

Up to you though.

well, it won't quite be:

return "", &missingConfigError{dir: dir, checkedParents: false}

because we need to run some logic in missingDevboxJSONError to make the error message read nicely.

🤔 if the struct implements the Error interface with the same logic in missingDevboxJSONError, would that work?

That's what I was thinking. Just make the Error method format the message however we want.

gcurtis · 2022-10-12T18:13:29Z

devbox.go

+			// we return a directory from this function
+			return filepath.Dir(absDir), nil
+		default:
+			return "", errors.Errorf("unhandled mode %v", mode)


What if we just assume that everything that isn't a directory is a file? If for some reason it is unsupported, then we'll get a read error later.

gcurtis · 2022-10-12T18:24:16Z

devbox.go

 }

+// findConfigDir
 func findConfigDir(dir string) (string, error) {


Should this go in config.go?

its added inside devbox.go to keep the CLI minimal. Moving this logic inside here enables devbox as a package to encapsulate all the logic and functionality (enabling reuse outside this CLI scenario).

I agree with keeping it in devbox. I was wondering if it should go in config.go within the same package.

savil

Can we just add some tests for the config searching behavior?

will do!

boxcli/add.go

savil · 2022-10-12T23:16:11Z

devbox.go

+		switch mode := fi.Mode(); {
+		case mode.IsDir():
+			if !plansdk.FileExists(filepath.Join(absDir, configFilename)) {
+				return "", missingDevboxJSONError(dir, false /*didCheckParents*/)


well, it won't quite be:

return "", &missingConfigError{dir: dir, checkedParents: false}

because we need to run some logic in missingDevboxJSONError to make the error message read nicely.

savil · 2022-10-12T23:16:23Z

devbox.go

+			// we return a directory from this function
+			return filepath.Dir(absDir), nil
+		default:
+			return "", errors.Errorf("unhandled mode %v", mode)


gcurtis · 2022-10-13T17:37:27Z

@savil there are a couple of options for testing findConfig.

Use t.TempDir() to create a test directory layout and give its path to findConfig.
Refactor findConfig to use an fs.FS. The CLI would pass in os.DirFS("/") and tests would pass in an fstest.MapFS. This might be a bit of work.

With either approach I think findConfig will need to be updated to take in a root path and a subpath. For example:

// or: findConfig(root fs.FS, dir string)
func findConfig(root, subdir string) (string, error) {
	root, _ := filepath.Abs(root)
	for subdir != "." {
		rooted := filepath.Join(root, cur)
		debug.Log("finding %s in dir: %s\n", configFilename, rooted)
		if plansdk.FileExists(filepath.Join(rooted, configFilename)) {
			return rooted, nil
		}
		subdir = filepath.Dir(subdir)
	}
}

// CLI
wd, _ := os.Getwd()
findConfig("/", wd)

// Tests
tmpDir := t.TempDir()
os.MkdirAll(filepath.Join(tmpDir, "a/b/c"))
os.WriteFile(filepath.Join(tmpDir, "a", "devbox.json"), []byte(`{}`), 0666)
findConfig(tmpDir, "a/b/c")

That will let you control where the search stops (cur will equal .) so the test doesn't go outside of the test directory.

- Update "use" and descriptions of commands - Error if both --config and comand arg are used for config-path - Change logic to do parent-directory search only when config path is not specified in flag or command-args - Shorten deprecation warning message - Lint fixes: replace unused cmd argument variables with underscore variables - Init command uses args: special case handling for init-cmd to use args. Ensure no warning printed.

- pass flag structs by value - make register a method on configFlags - improve shell help message

…path; add tests

savil force-pushed the savil/config-flag branch 2 times, most recently from 8eb948d to 2366111 Compare October 7, 2022 19:41

savil requested review from loreto and LucilleH October 7, 2022 19:47

savil marked this pull request as ready for review October 7, 2022 19:47

loreto reviewed Oct 8, 2022

View reviewed changes

savil commented Oct 10, 2022

View reviewed changes

savil force-pushed the savil/config-flag branch 4 times, most recently from 1237909 to a29ba81 Compare October 11, 2022 20:48

savil requested a review from loreto October 11, 2022 20:48

savil force-pushed the savil/config-flag branch from a29ba81 to 8b8505b Compare October 11, 2022 21:00

savil requested review from gcurtis and mikeland73 October 12, 2022 16:47

savil force-pushed the savil/config-flag branch from 8b8505b to 13c67a1 Compare October 12, 2022 17:13

savil mentioned this pull request Oct 12, 2022

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

Closed

gcurtis approved these changes Oct 12, 2022

View reviewed changes

savil commented Oct 12, 2022

View reviewed changes

savil added 3 commits October 13, 2022 14:01

[CLI] add --config flag

0c49027

Review fixes

cdca6fc

- pass flag structs by value - make register a method on configFlags - improve shell help message

savil force-pushed the savil/config-flag branch from 13c67a1 to 2b5831f Compare October 14, 2022 19:15

Move to config.go; split into two functions; rename dir variables to …

5c2655c

…path; add tests

savil force-pushed the savil/config-flag branch from 2b5831f to 5c2655c Compare October 14, 2022 19:19

savil merged commit c36a8db into main Oct 14, 2022

savil deleted the savil/config-flag branch October 14, 2022 19:26

[CLI] add --config flag #225

[CLI] add --config flag #225

Uh oh!

Conversation

savil commented Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How was it tested?

Uh oh!

savil commented Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

savil commented Oct 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

1

2

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loreto commented Oct 8, 2022

Uh oh!

savil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

savil commented Oct 12, 2022

Uh oh!

gcurtis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

savil commented Oct 7, 2022 •

edited

Loading

savil commented Oct 7, 2022 •

edited

Loading