Skip to content
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

[shellenv] add --recompute flag with default=true, while keep global shellenv's recompute flag with default=false #2013

Merged
merged 2 commits into from
Apr 25, 2024

Conversation

savil
Copy link
Collaborator

@savil savil commented Apr 20, 2024

Summary

Credit to @dax and #1963 for pointing out the need for this, and pioneering this solution.
This PR merely nudges the code to be a bit neater.

How was it tested?

% git diff
diff --git a/internal/boxcli/shellenv.go b/internal/boxcli/shellenv.go
index 991c2bec..8076e125 100644
--- a/internal/boxcli/shellenv.go
+++ b/internal/boxcli/shellenv.go
@@ -78,6 +78,9 @@ func shellEnvFunc(
        cmd *cobra.Command,
        flags shellEnvCmdFlags,
 ) (string, error) {
+       fmt.Printf("shellenv --recompute flag is %v\n", flags.recompute)
+       return "", nil
+
        env, err := flags.Env(flags.config.path)
        if err != nil {
                return "", err

compiled via devbox run build, and then:

 % devbox shellenv
shellenv --recompute flag is true

hash -r
 % devbox global shellenv
shellenv --recompute flag is false

hash -r

Copy link
Collaborator Author

savil commented Apr 20, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @savil and the rest of your teammates on Graphite Graphite

@savil savil requested a review from mikeland73 April 20, 2024 00:18
@savil savil marked this pull request as ready for review April 20, 2024 00:22
@savil savil requested a review from gcurtis April 23, 2024 00:18
…shellenv's recompute flag with default=false
@savil
Copy link
Collaborator Author

savil commented Apr 24, 2024

@mikeland73 @gcurtis ping

shellEnv := shellEnvCmd()
// For `devbox shellenv` the default value of recompute is true.
// Change the default value to false for `devbox global shellenv` only.
shellEnv.Flag("recompute").DefValue = "false"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is strictly needed , only for internal consistency. maybe add a comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The DefValue modifies the default value printed in the --help. Without this line, the help for the flag looks like:

% devbox global shellenv --help
...
  -r, --recompute            Recompute environment if needed (default true)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added comment to explain this need.

Comment on lines 35 to 37
if err := shellEnv.Flag("recompute").Value.Set("false"); err != nil {
panic(errors.WithStack(err))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add comment:

// This will never panic because internally it just does `strconv.ParseBool("false")` which is always valid

@savil savil merged commit 7034939 into main Apr 25, 2024
24 checks passed
@savil savil deleted the savil/recompute-flag branch April 25, 2024 15:58
Copy link

sentry-io bot commented Apr 29, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ *Generic Error: write flake_remove_nixpkgs.nix to file: <redacted fs.PathError>: go.jetpack.io/devbox/internal/shellgen in write... View Issue
  • ‼️ **Generic Error: <redacted errors.withStack>: <redacted usererr.combined> go.jetpack.io/devbox/internal/boxcli/usererr in... View Issue

Did you find this useful? React with a 👍 or 👎

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.

2 participants