Skip to content

[env] Add env flags to direnv commands #1354

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

Merged
merged 4 commits into from
Aug 10, 2023
Merged

[env] Add env flags to direnv commands #1354

merged 4 commits into from
Aug 10, 2023

Conversation

mikeland73
Copy link
Contributor

Summary

This allows users to generate .envrc files that use custom env variables like so:

devbox generate direnv -e foo=bar -e foo1=bar2
devbox generate direnv --env-file .env

Users can also manually modify .envrc files to add additional flags:

devbox generate direnv --print-envrc -e foo=bar

How was it tested?

Generated .envrc file with custom env flags. Then used direnv and verified environment was setup as expected.

Backward compatibility

Newly generated .envrc files that contain --env or --env-file flags will not be compatible with previous versions of devbox.

@Lagoja
Copy link
Contributor

Lagoja commented Aug 9, 2023

Just tested this. One issue with how this works is that the command will fail if the file passed with --env-file is not present. Since the .envrc will probably be checked into source control, but the .env file won't, this will cause the envrc to fail to load if the file is missing:

direnv allow
direnv: loading ~/src/devbox/.envrc
direnv: using devbox
Error: open .env.ghost: no such file or directory

A better solution might be to implement a "load if file exists" functionality, similar to direnv's dotenv_if_exists function:

https://direnv.net/man/direnv-stdlib.1.html#codedotenvifexists-ltdotenvpathgtcode

@savil
Copy link
Collaborator

savil commented Aug 10, 2023

Discussion clarified offline that the behavior won't break if the env-file is missing. Well, it won't break for direnv (devbox generate direnv --env-file fo), but will break if env file is missing for devbox run --env-file foo

Copy link
Collaborator

@savil savil left a comment

Choose a reason for hiding this comment

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

@mikeland73 where is the logic that makes it forgiving if --env-file file is missing?

Also - I don't understand why the shellenv hash change is related, and how it works...

// If hash is already set, the env is up to date, no need for wrappers.
if args.ShellEnvHash == os.Getenv(args.ShellEnvHashKey()) {
return nil
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

wait, this change is quite unrelated to this PR. Can we move it out?

I don't think this makes sense. We need the wrappers because the shellenv environment may change after the wrappers have been generated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a bad change that snuck in.

Copy link
Collaborator

@savil savil left a comment

Choose a reason for hiding this comment

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

acceping to unblock, assuming:

  1. the change to make --env-file forgiving for direnv is added
  2. the wrapper change is reverted

Copy link
Collaborator

@savil savil left a comment

Choose a reason for hiding this comment

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

nice!

@mikeland73 mikeland73 merged commit 1052c4e into main Aug 10, 2023
@mikeland73 mikeland73 deleted the landau/env2 branch August 10, 2023 23:32
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.

3 participants