-
-
Notifications
You must be signed in to change notification settings - Fork 809
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 support for environment files #598
base: master
Are you sure you want to change the base?
Conversation
At times, it is necessary or at least easier to set environment variables using a file. Add a function that augments the environment of the spawned process with the provided variables. Signed-off-by: Moritz Poldrack <git@moritz.sh>
On Sun 02 Jun 2024 05:11:53, ccoVeille wrote:
+func (e *Engine) modifyEnvironment(c *exec.Cmd) {
By bare return, I meant using `return c`everywhere and not `return`
That is completely missing the point of a bare return, I think. If the
function signature does not specify any returns, of course a bare return
is the way to… well… return. Bare returns, at least as I understand it
are referring to:
```go
// bare return, i. e. bad
func (e *Engine) modifyEnvironment(c *exec.Cmd) (err error) {
err = errors.New("I feel bare")
return
}
// non-bare return, i. e. better
func (e *Engine) modifyEnvironment(c *exec.Cmd) error {
return errors.New("I feel bare")
}
```
To be fair, not the best example, but it gets the point across. Since it
wasn't using a bare return this doesn't apply. I am still happy to
report that the added return is not bare :)
…--
Moritz Poldrack
https://moritz.sh
Not intended to diagnose, treat, cure or prevent any disease.
|
OS handling of environment variables and Go stdlib.
…--
Moritz Poldrack
https://moritz.sh
No alcohol, dogs or horses.
|
why do u just pass env var like this:
|
@xiantang I was also wondering when I read the PR for the first time. For me the environment variables are what they are supposed to be something outside the binary that exists prior the binary is launched, they are added by the shell. I was surprised by what @mpldr added, but I was like, never mind 🤷, but as you raise the point 🤨 too. I feel like my first thought was right. I wouldn't expect this to be added to It's usually the role of:
|
mainly two reasons:
Since air acts as the launcher (it calls |
What about using the air config file to set the env variables, so not the location of an env file? |
That was an approach I was considering, but ultimately abandoned it because .env files or some variation of them are usually already present. And having two places to maintain environment variables might be a tough sell :) |
Thanks for your reply. It won't change the points I already raised, it's surprising to see such implementation. For me, it's over the role of an app to do that I look at what supervisord was doing and it was providing such a feature and the reply and arguments were very similar But anyway, why not… but I would discourage you doing it. |
I'm still not convinced, but it's not a problem 😅. I'm a random voice among others. I can live with this feature being in the app, but I won't use it. But if you want to keep the feature please look at #598 (comment) |
At times, it is necessary or at least easier to set environment variables using a file.
Add a function that augments the environment of the spawned process with the provided variables.
Tested to work on Linux. Not sure on how to write a test for it without either passing in the content as a reader or basically testing stdlib IO functions.