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

Set individual environment variables for build #99

Merged
merged 6 commits into from
Mar 26, 2019

Conversation

scothis
Copy link
Contributor

@scothis scothis commented Feb 14, 2019

pack allows setting build-time environment variables via a file, but not
as individual values on the cli. This PR adds the ability to set one (or
more) environment variables directly as cli switches.

The same semantics are preserved from --env-file where a value can be
forwarded from the current environment if a variable name is specified
without a value. The new flags can be used to augment or override
specific values from a file.

The BuildFlags struct now includes an Env field in addition to EnvFile.
The BuildConfig struct EnvFile field has been renamed to Env for
consistency as the field contains the parsed values and not a file.

Refs buildpacks/roadmap#25

@scothis
Copy link
Contributor Author

scothis commented Feb 14, 2019

A little background context.

riff currently uses a riff.toml file to pass configuration to function buildpacks. This file is generated based on values provided to the riff cli. Since this config file is not actually part of the project, it's awkward to write a file to the filesystem, do some work and then remove that file. It would be nicer to specify this config as environment variables.

pack allows setting build-time environment variables via a file, but not
as individual values on the cli. This PR adds the ability to set one (or
more) environment variables directly as cli switches.

The same semantics are preserved from --env-file where a value can be
forwarded from the current environment if a variable name is specified
without a value. The new flags can be used to augment or override
specific values from a file.

The BuildFlags struct now includes an Env field in addition to EnvFile.
The BuildConfig struct EnvFile field has been renamed to Env for
consistency as the field contains the parsed values and not a file.

Refs buildpacks/roadmap#25

Signed-off-by: Scott Andrews <sandrews@pivotal.io>
@scothis
Copy link
Contributor Author

scothis commented Feb 22, 2019

Merged master and fixed a conflict with build config changes.

cc @ekcasey

@ekcasey
Copy link
Member

ekcasey commented Mar 11, 2019

@scothis I was under the impression that riff was importing github.com/buildpack/pack as a library using the pack.Build(...) function to run builds. In which case, I would imagine the correct solution for this problem is to add a map of env vars as a param to pack.Build. In that case riff should be able to construct that map as desired (no file required).

Is riff shelling out to the pack cli? I am not opposed to supporting a --env flag but to better enable library consumers it's important to us to know where users are integrating. In the near future we plan to make everything except a deliberately exposed library internal and thus not importable.

@scothis
Copy link
Contributor Author

scothis commented Mar 22, 2019

@ekcasey yes, riff embeds pack rather than shelling out. On its own, this PR is not enough to fully solve riff's needs, but it provides a large step towards scratching that itch. I'll open another PR that takes a baby step towards creating a programatic API that exposes the same capabilities.

This PR is updated to resolve conflicts.

@ekcasey
Copy link
Member

ekcasey commented Mar 25, 2019

@scothis Thanks for the explanation. If we think cli users will value having a --env flag in addition to an --env-file, flag this is a good step.

I think the only thing missing here is acceptance test coverage. If we are exposing a new flag we should add coverage in acceptance/acceptance_test.go, just like we do with --env-file.

Signed-off-by: Scott Andrews <sandrews@pivotal.io>
@scothis
Copy link
Contributor Author

scothis commented Mar 25, 2019

@ekcasey added an acceptance test and manually verified that all the tests are actually passing

@ekcasey ekcasey merged commit 4958020 into buildpacks:master Mar 26, 2019
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.

None yet

2 participants