SHIP-0004: Build Environment Variables

[coreydaley]

Changes

This pull request will give developers the abilty to add environment variables to build strategy steps. Conversely, this proposal will also give build strategy authors the ability to fix the values of environment variables in build steps or provide non-empty default values.

Submitter Checklist

• Includes tests if functionality changed/was added
• Includes docs if changes are user-facing
• Set a kind label on this PR
• Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

Developers can now add Environment Variables to the Spec section of their Builds and BuildRuns.
The Environment Variables will be added to all Steps of the chosen BuildStrategy, though any conflicts
will result in a failed TaskRun.

[qu1queee]

@coreydaley can we close this one? see shipwright-io/community#10 ( is already merged )

[coreydaley]

@coreydaley can we close this one? see shipwright-io/community#10 ( is already merged )

This is the implementation for shipwright-io/community#10 ...

[qu1queee]

@coreydaley my bad, thanks for the clarification

[coreydaley]

/retest

[coreydaley]

/assign @adambkaplan @gabemontero @otaviof
I believe that this is ready for a first round of reviews, thanks!

[coreydaley]

/assign @SaschaSchwarze0

[adambkaplan]

Generally looks good, but given this is a substantial feature this PR needs:

1. Doc updates for our Build and BuildRun APIs.
2. A release note block in the PR description.

[coreydaley]

Generally looks good, but given this is a substantial feature this PR needs:

1. Doc updates for our Build and BuildRun APIs.

2. A release note block in the PR description.

Working on the docs update already.

[coreydaley]

Documentation update: shipwright-io/website#53

[gabemontero]

a couple of bigger macro items @coreydaley @adambkaplan that came to my mind, including perhaps missing a key element of type during the SHIP review

[coreydaley]

Looks like a networking issue:

error creating build container: Error reading blob sha256:e571554b15ee6f4301b3e1cf58735a1504380734dade095784e14197eea233f8: Get "https://cdn02.quay.io/sha256/e5/e571554b15ee6f4301b3e1cf58735a1504380734dade095784e14197eea233f8?Expires=1632847588&Signature=OG~knNhsUflW2XAC3MjGAicDdiEKMxMWlB4vIjF4JkI3WBp5O7sp2ffRULJHvK1H~gBzH7wH4mQe~5rRKlwhGgSNCOT1PZ-Edc3~oBCVMbwNEi2UlI~6gawEvxgcGzGN9Uj6iDj~dE1jjAzqPVL~2zh1eIjju2qUsu-~~vxzOpRoPvI9gTd8F7ynj0fj-ZFbUmrt8lKmooOjUcb82O5KQ~lQ4PitDoPtsRNt0RJx96veZG~jVHcuMfACRRZQJeYqbZawLjC4voGHBWF0DbIutUIgX6~zk1GNzviBHPpwfJi9SaeBHD6WLAe8VIgZeHNGczWW0YYywA925yUqPIsVeQ__&Key-Pair-Id=APKAJ67PQLWGCSP66DGA": net/http: TLS handshake timeout

@adambkaplan Can I get a retest?

[adambkaplan]

@coreydaley for future reference I think you can mention @shipwright-io/build-reviewers, and anyone on that team will be able to rerun any GitHub action by clicking on the details.

You may be also able to re-run the GitHub action as the PR author since technically the tests run against your fork.

[coreydaley]

@coreydaley for future reference I think you can mention @shipwright-io/build-reviewers, and anyone on that team will be able to rerun any GitHub action by clicking on the details.

You may be also able to re-run the GitHub action as the PR author since technically the tests run against your fork.

I will keep that in mind. I was not able to rerun the tests.

[coreydaley]

I don't think that adding additional documentation to support valueFrom with the EnvVars would be an issue, and it should flow through to tekton seamlessly. Additional supporting tests should not be an issue either.
However, the shipwright/cli would not be able to support adding environment variables utilizing valueFrom to Builds or BuildRuns, I also don't think that OpenShift currently supports that through the oc cli either.

[gabemontero]

I don't think that adding additional documentation to support valueFrom with the EnvVars would be an issue, and it should flow through to tekton seamlessly. Additional supporting tests should not be an issue either. However, the shipwright/cli would not be able to support adding environment variables utilizing valueFrom to Builds or BuildRuns, I also don't think that OpenShift currently supports that through the oc cli either.

the meets min for me would be doc and tests that address the various valueFrom paths with k8s env vars ... if those are added, and we are all good with supporting those scenarios, I'm good

integration into shp i.e. cli is a stretch item that could either never be addressed or addressed at a later point based on customer feedback, though the fact that openshift has k8s corev1 env vars for 7 years now and nobody has asked for cli enablement there is telling IMO

so yeah @coreydaley pending any comeback from @adambkaplan about gettnig simpler here wrt env vars, start adding the additional env var permutations and we'll go from there

[SaschaSchwarze0]

Thanks. Good work.

[coreydaley]

FYI, I am still working on the valueFrom stuff that was mentioned by @gabemontero during his review, and have not pushed those updates as of yet.

[SaschaSchwarze0]

FYI, I am still working on the valueFrom stuff that was mentioned by @gabemontero during his review, and have not pushed those updates as of yet.

Thanks, was assuming something like this. You may prefix your PR title with WIP in such cases.

[gabemontero]

a couple of bigger macro items @coreydaley @adambkaplan that came to my mind, including perhaps missing a key element of type during the SHIP review

ok my bigger macro items have been addressed ... I will thus

/approve

given the collaboration I see in the commits between @coreydaley @HeavyWombat @shahulsonhal, as well as the review from @SaschaSchwarze0 I am going to defer the lgtm to one of them. Aside from their in depth involvement, it allows for the cross team etc. sign off we prefer.

thanks everyone

[openshift-ci]

[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:

• OWNERS [gabemontero]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[SaschaSchwarze0]

Looks good, one mandatory item remaining regarding for which steps environment variables are set in the TaskSpec.

Beside that, two bonus items that we can also do in a later PR:

1. In the build.md and buildrun.md, your examples for valueFrom are all working I guess, but are not covering a useful scenario, I think. A useful scenario could be to get an environment variable value for NPM_TOKEN from a secret in the context of a Buildpacks or s2i build.
2. In the Build validation, we could add another check that verifies that an environment variable name does not colide with any step in the build strategy steps.

EDIT: and a third bonus item would be the validation of secrets and configmaps. We today validate that a referenced source secret, and output secret exist. We could validate the same for any secrets or configmaps referenced in environment variables.

[SaschaSchwarze0]

My comment might be stupid, but I am coming from a decade of Java development where by-mistake-mutations-of-cached-objects was a quite often found issue and it was always a pain. Might not even apply here, so, if you tell me you're fully confident that your code is safe, I am also fine to take what you have.

[SaschaSchwarze0]

/lgtm