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

Nice way to handle env variable? #80

Closed
Prizrako opened this issue Apr 5, 2021 · 25 comments · Fixed by #208
Closed

Nice way to handle env variable? #80

Prizrako opened this issue Apr 5, 2021 · 25 comments · Fixed by #208

Comments

@Prizrako
Copy link

Prizrako commented Apr 5, 2021

Some methods to set environment varialbes for single command, instead of this:

os.Setenv("XB_AUTHOR_DATE", dateStr)
os.Setenv("XB_COMMITTER_DATE", dateStr)

Maybe in old shell way:

XB_AUTHOR_DATE="2021-04-05" XB_COMMITTER_DATE="2021-04-05" xb_push xxxx
@No-one-important
Copy link
Contributor

would
script.Env("variable", "value").Exec("command")
be a suitable solution

@bitfield
Copy link
Owner

bitfield commented Apr 3, 2022

I'm not seeing where this adds much value over, for example:

script.Exec("sh -c 'export MSG=hello; echo $MSG'").Stdout()

@No-one-important
Copy link
Contributor

on windows that syntax is not possible

@bitfield
Copy link
Owner

bitfield commented Apr 3, 2022

That's a fair point, but what about os.Setenv, for example?

@No-one-important
Copy link
Contributor

It works but I think it would cleaner to have it inline and builtin to the module.

@bitfield
Copy link
Owner

bitfield commented Apr 3, 2022

Useful abstractions, as John Ousterhout says, should be deep: they should conceal lots of functionality behind a simple API. Is there any way we could do more than simply call os.Setenv? Some example programs that need to set their environment would be useful. What about reading an envfile, for example?

@josegonzalez
Copy link

I think it might be nice to not use os.Setenv() and instead only set them for a Pipeline - maybe something like a wrapper around script.Exec("sh -c 'export KEY=value; true'"), but for potentially multiple key/value pairs? In this way, we scope environment variables to just the pipeline rather than the entire golang process that is invoking/utilizing the pipeline.

Note that the wrapper would probably need to properly escape the environment variables, so it wouldn't be as simple as a fmt.Sprintf() call. Maybe thats enough deepness for it to be implemented?

@bitfield
Copy link
Owner

That sounds good, @josegonzalez—can you write a short piece of example code to show what you mean?

@josegonzalez
Copy link

Maybe something like the following:

environ := map[string]string{
  "HASURA_GRAPHQL_JWT_SECRET": "{\"type\": \"HS256\", \"key\": \"256BITKEYHERE$\"}"
}
contents, err := script.Env(env).Exec("env")

@bitfield
Copy link
Owner

Okay, but this looks pretty similar to:

os.Setenv("HASURA_GRAPHQL_JWT_SECRET", `{"type": "HS256", "key": "256BITKEYHERE$"}`)
contents, err := script.Exec("env")

Can you describe what problem you think an Env method on the pipe would be solving here?

@No-one-important
Copy link
Contributor

os.Setenv would be global to the entire script this would allow it be local to the pipe

@bitfield
Copy link
Owner

Okay, but why is that important? Can you think of a situation where you'd want to set some environment variable for a command run by a pipe, but not for the program as a whole?

@No-one-important
Copy link
Contributor

I'm not sure of the usecases but it just allows the same flexibility that the bash syntax allows but for all OSs

@bitfield
Copy link
Owner

Well, if there are no use cases for something, that's just another way of saying we don't need it, isn't it? There are lots of feature ideas that aren't bad in themselves—they're great features! It's just that we don't need them, so we don't add them.

@bitfield
Copy link
Owner

bitfield commented Jun 2, 2022

Closing pending real use cases.

@bitfield bitfield closed this as completed Jun 2, 2022
@lambrospetrou
Copy link

lambrospetrou commented Aug 13, 2024

@bitfield I was actually looking at this today for a use-case I have... As part of the Skybear.NET platform, I am running a few subprocesses per request/job. I want each of those command Execs to have their own environment, since I pass account specific things and I don't want to put them on the global process level environment.

Having a way to pass environment variables to modify the cmd.Env of the command created byNewPipe().XYZ().Exec("my command") would be awesome.

Right now, I am probably going to revert to using the os package since I cannot do it with script, which is sad since I love the UX of this package.

I could do the method above to put everything into the command itself with exports, but that's going to be cumbersome and error-prone to maintain, especially for "unsetting variables".

@bitfield
Copy link
Owner

@lambrospetrou that's great! Could you comment with a sketch of the kind of program you'd like to write, showing a convenient way to set the required environment variables? I don't yet have a good idea of the right API for this.

@bitfield bitfield reopened this Aug 13, 2024
@lambrospetrou
Copy link

lambrospetrou commented Aug 13, 2024

Without spending ages thinking about it, my first intuition would be something like below.

Examples

I am using the Go definition of an environment as []string but your version can adapt accordingly if you don't like that (e.g. using a map).

Entirely new environment

Similar to the WithStdout/WithStderr helpers.

customEnv := []string{"PATH=/something", "VAR=something-else"}
script.NewPipe().WithEnv(customEnv).Exec("my long command")

Append to the default environment

By default, right now the command will inherit the parent process environment. So the WithExtraEnv helper will explicitly do that, by taking the os.Environ() and calling append(os.Environ(), extraEnv...) to add the extra provided environment variables.

extraEnv := []string{"VAR2=something2"}
script.NewPipe().WithExtraEnv(extraEnv).Exec("my long command")

Unset environment variables

This is still tricky, but it's hard to automagically do by nature. With the above two options though someone can easily either provide the complete environment they want, hence they can just generate the environment they need (even taking os.Environ() and manipulating it), or overwrite existing variables by passing empty values since the last value wins (see example https://go.dev/play/p/inKF-5GswlK ).

References

@bitfield
Copy link
Owner

bitfield commented Aug 13, 2024

Rather than have two methods, I'd prefer to have one that behaves the same way as setting cmd.Env: its argument completely replaces the current environment.

That way, if you want to control the whole environment:

script.NewPipe().
  WithEnvironment([]string{"FOO=bar"}).
  Exec("myprog")

That solves the problem of how to unset variables. On the other hand, to add extra variables to the current environment, you can use append as in the stdlib example.

script.NewPipe().
  WithEnvironment(append(os.Environ(), "FOO=bar")).
  Exec("myprog")

Would this API work for your use case?

@lambrospetrou
Copy link

Yeap! This can work.

@mahadzaryab1
Copy link
Contributor

mahadzaryab1 commented Aug 14, 2024

@bitfield are you looking for someone to work on this? i've never contributed to an open source project and this issue seems like a fun one to work on so I'd love to contribute here if possible.

@bitfield
Copy link
Owner

@mahadzaryab1 sure, all contributions are very welcome!

@josegonzalez
Copy link

Ah I have a use-case, sorry for not posting.

Basically I have a web process that can - in a single api call - spawn a bunch of processes serially with different environment variables. I'd like to not leak the current process state/have to track which variables to unset and reset at a later date.

@mahadzaryab1
Copy link
Contributor

thanks @bitfield! i'll start working on this right away

@mahadzaryab1
Copy link
Contributor

hi @bitfield! i've got a PR open at #208 and would appreciate any feedback you have

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 a pull request may close this issue.

6 participants