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
Allow env variable interpolation in Container.withEnvVariable #7171
Comments
I remember that withEnvVariable has an option to expand variable here https://github.com/dagger/dagger-go-sdk/blob/v0.11.1/dagger.gen.go#L1063. Is it similar use case? |
Or it should start expand by default? |
The `withEnvVariable` API will expand shell env variable by default Closes dagger#7171 Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Yeah, this exists just isn't the default. More annoying in Go because optionals are verbose: Go c.WithEnvVariable("PATH", "$HOME/bin:$PATH", ContainerWithEnvVariableOpts{
Expand: true,
}) Python ctr.with_env_variable("PATH", "$HOME/bin:$PATH", expand=True) TypeScript ctr.withEnvVariable("PATH", "$HOME/bin:$PATH", {expand: true}) The best argument in favor of flipping the default is perhaps users don't trip up when they assume it should work like that off the shelve. |
Wow I had no idea this existed! Then yes, I am arguing for 1) making it the default, and 2) considering adding it to other Container functions as well. |
Making it do this by default feels like a potential footgun to me, thinking of cases where you might pass a value in from a user that happens to contain a We shouldn't have to worry about special characters in secrets, at least, since those are handled separately I can see the argument that not doing it is a footgun, but it's a different kind of footgun since you'll notice behavior not happening and go fix it, rather than potentially not notice a latent bug until it blows up because of an unexpected value (Pretty low conviction on this, just think it warrants more bikeshedding) |
The `withEnvVariable` API will expand shell env variable by default. And add `noExpand` option to do not expand when it's set to `true`. The `expand` option is now deprecated but still compatible with the previous version. Updates dagger#7171 Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
The `withEnvVariable` API will expand shell env variable by default. And add `noExpand` option to do not expand when it's set to `true`. The `expand` option is now deprecated but still compatible with the previous version. Updates dagger#7171 Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
I removed the However, we can close this issue since we already support variable interpolation |
@shykes still argues for making this the default though:
So I'll let him reopen if he wants to push for that, but @vito has concerns that I somewhat share and asks for more bikeshedding. |
Yeah, just adding my two cents here that I also agree with @vito here - it feels like a possibly annoying breaking change (so probably a candidate for v0.12.0 instead of v0.11.X IMO). I'm also not entirely convinced it should be the default, I think each command should try and do as little magic by default as possible, and I don't think at least I would expect expansion by default (e.g. I think things like this are one of the things that makes bash-programming such a pain - I'd like to avoid that as much as possible). |
Problem
Dagger has no direct equivalent to this simple Dockerfile line:
ENV PATH=foo:${PATH}
.Solution
Add an optional argument
Container.withEnvVariable
to allow interpolating the current container's env variables into string arguments. And possibly, add it to otherContainer
functions as well.The default for this new argument should be
true
, I think. In other words, interpolation should happen by default.Then this would work out of the box:
The text was updated successfully, but these errors were encountered: