-
Notifications
You must be signed in to change notification settings - Fork 112
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
Extend the Buildpack strategy to work with the environment variables #906
Extend the Buildpack strategy to work with the environment variables #906
Conversation
We noticed a shortcoming of the Buildpacks build strategy: as we bypass pack, the environment variables are not working. Extend the buildstrategy_buildpacks-v3_cr.yaml with the script to process environment variables. Add a test case to verify the environment variables working with Buildpack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also update the other build strategy files for buildpacks (heroku and the namespaced samples). I'd say no additional test necessary for them assuming we run the exact same code.
|
||
for env in "${envs[@]}"; do | ||
IFS='=' read -r key value string <<< "$env" | ||
if [[ "$key" != "" && "$value" != "" ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not ignore empty values. There might be theoretic use cases where "not present" vs "empty string" makes a difference.
if [[ "$key" != "" && "$value" != "" ]]; then | |
if [[ "$key" != "" ]]; then |
IFS='=' read -r key value string <<< "$env" | ||
if [[ "$key" != "" && "$value" != "" ]]; then | ||
path="${ENV_DIR}/${key}" | ||
echo "--> Writing ${path}..." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be very noisy. I personally would remove the echo
s.
I do not feel good about having a |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gabemontero The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@coreydaley Thanks. I have replaced |
/retest |
871cc1f
to
3d6b833
Compare
3d6b833
to
afb462c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Changes
We noticed a shortcoming of the
Buildpacks
build strategy: as we bypass pack, the environment variables are not working.Extend the
buildstrategy_buildpacks-v3_cr.yaml
with the script to process environment variables.Add a test case to verify the environment variables working with the
Buildpack
build strategy.Note: This PR has a dependency with shipwright-io/sample-go#6. The test cases will only pass after merging the shipwright-io/sample-go#6.
Submitter Checklist
See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.
Release Notes