Skip to content

Add expand option when adding environment variables#5160

Merged
helderco merged 2 commits into
dagger:mainfrom
helderco:helder/dag-1567-expand-environment-variables
May 25, 2023
Merged

Add expand option when adding environment variables#5160
helderco merged 2 commits into
dagger:mainfrom
helderco:helder/dag-1567-expand-environment-variables

Conversation

@helderco
Copy link
Copy Markdown
Contributor

@helderco helderco commented May 20, 2023

Fixes #5140

Note: This only applies to withEnvVariable.

Common example is to prepend to $PATH:

WithEnvVariable("VIRTUAL_ENV", "/opt/venv").
WithEnvVariable(
	"PATH",
	"${VIRTUAL_ENV}/bin:$PATH",
	dagger.ContainerWithEnvVariableOpts{
		Expand: true,
	},
).

In Python:

ctr
    .with_env_variable("VIRTUAL_ENV", "/opt/venv")
    .with_env_variable("PATH", "${VIRTUAL_ENV}/bin:$PATH", expand=True)

@helderco helderco requested review from marcosnils and vito May 20, 2023 12:54
@helderco
Copy link
Copy Markdown
Contributor Author

I considered keeping the utils private by moving the code from core/schema to core but decided this was simpler.

@helderco
Copy link
Copy Markdown
Contributor Author

I've put the refactoring and the flag addition in separate commits to help in review.

@nipuna-perera
Copy link
Copy Markdown
Contributor

nipuna-perera commented May 22, 2023

Can this be the default behavior? IMO it's more intuitive that way. I initially assumed it worked that way. Majority of the steps of my current pipeline uses env/secret vars that need to be expanded for CLI commands so having to add the new opts for each withExec is not ideal. Could this be possibly enabled at the client level so I can set it once?

@helderco
Copy link
Copy Markdown
Contributor Author

Can this be the default behavior? IMO it's more intuitive that way. I initially assumed it worked that way.

See @vito's reasoning in #5001 (reply in thread).

@shykes wdyt?

@helderco helderco force-pushed the helder/dag-1567-expand-environment-variables branch from ee849b9 to a100efd Compare May 22, 2023 21:23
@helderco
Copy link
Copy Markdown
Contributor Author

Majority of the steps of my current pipeline uses env/secret vars that need to be expanded for CLI commands so having to add the new opts for each withExec is not ideal.

@nipuna-perera This PR adds the expand option only to withEnvVariable. If you need to expand in withExec, just use a shell:

- WithExec([]string{"echo", "$PATH"})
+ WithExec([]string{"sh", "-c", "echo $PATH"})

@helderco helderco force-pushed the helder/dag-1567-expand-environment-variables branch from a100efd to 3e7366b Compare May 22, 2023 21:52
@nipuna-perera
Copy link
Copy Markdown
Contributor

Majority of the steps of my current pipeline uses env/secret vars that need to be expanded for CLI commands so having to add the new opts for each withExec is not ideal.

@nipuna-perera This PR adds the expand option only to withEnvVariable. If you need to expand in withExec, just use a shell:

- WithExec([]string{"echo", "$PATH"})
+ WithExec([]string{"sh", "-c", "echo $PATH"})

Ohh I misunderstood! Now, an expand on the withExec would also be very handy. I have quite elaborate shell commands that I have to define outside of the withExec to do string substitution. That makes the withExec less readable.

@helderco helderco force-pushed the helder/dag-1567-expand-environment-variables branch from 3e7366b to fcb8f39 Compare May 22, 2023 23:22
@helderco helderco requested a review from sipsma May 24, 2023 23:43
helderco added 2 commits May 24, 2023 23:57
Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
@helderco helderco force-pushed the helder/dag-1567-expand-environment-variables branch from f021e34 to 15667df Compare May 24, 2023 23:58
@helderco helderco merged commit e171dc4 into dagger:main May 25, 2023
@helderco helderco deleted the helder/dag-1567-expand-environment-variables branch May 25, 2023 09:29
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.

Expand environment variables in withEnvVariable values

3 participants