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

Add dotenv support to dispatch run and dispatch login commands #57

Merged
merged 5 commits into from
May 22, 2024

Conversation

chicoxyzzy
Copy link
Contributor

@chicoxyzzy chicoxyzzy commented May 21, 2024

This PR adds a --env-file option support similar to recently introduced Node.js's --env-file.
Later, when we can add this file to dispatch init under some flags to autogenerate this file and avoid explicit export of environment variables for local development. It's also could be handy for adding rules like make run-local which run project against local development environment and other situation.

⚠️ Variables from the file do not override the ones passed with command line (default github.com/joho/godotenv behavior). I think this correct behavior, but it worth disacussing
⚠️ I've realized to only override env variables on PreRunE Cobra hook to avoid FS operations on dispatch commands other than run and login to make them work faster, but I can change that to always load config as @achille-roussel proposed. The --env-file is a global option at the moment. We may want to make it non-global or leave it global and always load config. cc @achille-roussel The option is global and it always loads a file when provided

@chicoxyzzy
Copy link
Contributor Author

I think that it would be super useful to add test coverage for the feature

Copy link
Contributor

@achille-roussel achille-roussel left a comment

Choose a reason for hiding this comment

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

If I'm understanding correctly, the priority order will be:

  • command line
  • local env vars
  • .env file

Is that right?

I would prefer to either make the option local to the commands it applies to, or always load the file if it's a global option. I like the "always available" if possible, it keeps the mental model simple, but also fine if we pick the other approach as a first step.

cli/env_file.go Outdated Show resolved Hide resolved
@chicoxyzzy
Copy link
Contributor Author

If I'm understanding correctly, the priority order will be:

  • command line
  • local env vars
  • .env file

Is that right?

Yes, that's correct

@chicoxyzzy
Copy link
Contributor Author

I would prefer to either make the option local to the commands it applies to, or always load the file if it's a global option. I like the "always available" if possible, it keeps the mental model simple, but also fine if we pick the other approach as a first step.

The option is always global now

Copy link
Contributor

@achille-roussel achille-roussel left a comment

Choose a reason for hiding this comment

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

Let's move env_file.go into config.go and get this merged!

Nice work 👏

@chicoxyzzy
Copy link
Contributor Author

chicoxyzzy commented May 22, 2024

I've just made last changes and tested. It's ready for merge now

@chicoxyzzy chicoxyzzy merged commit d8d484e into main May 22, 2024
2 checks passed
@chicoxyzzy chicoxyzzy deleted the dotenv_support branch May 22, 2024 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants