SHIP-0004: Build Environment Variables #27

coreydaley · 2021-06-23T19:59:07Z

Changes

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

Adds the ability to specify environment variables for Builds and BuildRuns as a key=value pair via the command line using the --env flag.
Environment variables specified for a BuildRun will override those in the Build.

Example:  
    $ shp build create my-build --env VAR_1=value-1  
    $ shp buildrun create my-buildrun --env VAR_2=value-2

coreydaley · 2021-09-27T19:02:58Z

/hold
depends on shipwright-io/build#817 and removing line from go.mod

gabemontero · 2021-09-28T15:46:13Z

Changes

Submitter Checklist
* [x]  Includes tests if functionality changed/was added

* [ ]  Includes docs if changes are user-facing

* [ ]  [Set a kind label on this PR](https://prow.k8s.io/command-help#kind)

* [ ]  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

release note formattting is not quite right ... nothing is rendoring underneath the heading as I currently view it

gabemontero · 2021-09-28T15:47:48Z

otherwise, since I see no one assigned, am I correct @coreydaley that in addition to the hold you have here, this is still a work in progress item, and we should hold off on reviewing?

Or should I add this to the queue and dive in?

thanks

coreydaley · 2021-09-28T16:46:17Z

@gabemontero

I have updated the Release Notes section.
Yes, you can go ahead and do a review, keeping in mind the comments that you left on shipwright-io/build and that we are waiting on @adambkaplan to make a decision in regards to the afformentioned.

coreydaley · 2021-09-28T16:59:58Z

These tests won't pass until shipwright-io/build#817 merges

gabemontero

Yeah pending the type for the env var array per my comments in your associated build PR nothing immediate that I see @coreydaley

I did ask a question around the kind / local registry thing ... maybe a change needed there based on the answer (but I'm guessing no)

and lots of useful libs around BATS and the e2e's .... I plan to wait until all that merges so I can build off of it for adding an e2e for ship build run <name> --follow :-)

gabemontero · 2021-09-28T18:16:23Z

test/e2e/e2e.bats

+	buildrun_name=$(random_name)
+
+	# create a Build with two environment variables
+	run shp build create ${build_name} --source-url=https://github.com/shipwright-io/sample-go --output-image=my-image --env=VAR_1=build-value-1 --env=VAR_2=build-value-2


@otaviof @coreydaley - I have not tried it yet, but perhaps you all have. With the kind/local registry stuff we are employing, is it correct to say that by default we do NOT need credentials to push images to it?

And hence why I don't see cred creation here?

Yes, we can assume the registry won't have any authentication, since we are in complete control of the container registry in use after #43 👍🏼

gabemontero · 2021-09-28T18:19:16Z

/approve

gabemontero · 2021-09-28T18:19:56Z

/hold

just in case, as we need to vendor in the associated build PR

gabemontero · 2021-09-29T19:07:28Z

@gabemontero

* I have updated the Release Notes section.

* Yes, you can go ahead and do a review, keeping in mind the comments that you left on shipwright-io/build and that we are waiting on @adambkaplan to make a decision in regards to the afformentioned.

some cross referencing with shipwright-io/build#817 (comment) and shipwright-io/build#817 (comment)

summary

@coreydaley thinks he'll be able to add all the k8s corev1.envvar *valuefrom related doc and tests, so his change will continue to use that k8s type
most likely this cli pr will not take on creating env vars from those supplied sources ... we'll track that as a potential add on based on user feedback, with the history that with the analogous support in openshift builds, we never took on cli level support for adding valuefrom env vars

otaviof

Some comments left on the pull-request, and I'm having issues with the Go modules changed. As per:

$ go mod vendor
go: github.com/shipwright-io/[email protected] (replaced by github.com/coreydaley/[email protected]): version "v0.0.0-20210927153915-3175d47a83e4" invalid: unknown revision 3175d47a83e4
go: downloading github.com/coreydaley/shipwright-io-build v0.0.0-20210927153915-3175d47a83e4
go: downloading github.com/spf13/cobra v1.2.1
go: downloading k8s.io/apimachinery v0.20.6
go: downloading k8s.io/client-go v0.20.2
go: downloading k8s.io/api v0.20.6
go: downloading github.com/onsi/gomega v1.10.3
go: downloading k8s.io/cli-runtime v0.20.2
go: downloading github.com/texttheater/golang-levenshtein/levenshtein v0.0.0-20200805054039-cae8b0eaed6c
go: downloading k8s.io/utils v0.0.0-20210111153108-fddb29f9d009
go: github.com/shipwright-io/[email protected] (replaced by github.com/coreydaley/[email protected]): version "v0.0.0-20210927153915-3175d47a83e4" invalid: unknown revision 3175d47a83e4

Same with go mod tidy, for the record.

otaviof · 2021-10-04T08:42:26Z

test/e2e/e2e.bats

@@ -2,25 +2,82 @@

 source test/e2e/helpers.sh

+setup() {
+	load 'helpers/bats-support/load'


As much as I like the syntax and the practical effects of the "bats-support" helper functions, I don't like the idea of copying the whole project. I think we could create our own helper functions and make use local tools like grep, awk and others.

That seems a bit silly to basically re-write a library that is provided by the framework that we want to use.
I would be open to adding it as a submodule.

hmmm ... I see both sides here

don't know the answer to this question: even though bats is not a go project, can we use go mod to vendor the repo in, and then load from the vendored location?

That seems a bit sketchy to me, I think I would rather the submodule.

sorry multitasking too much this AM ... can you clarify / further explain what you mean by submodule ?

looking at @coreydaley 's 2150530 I actually agree with him @otaviof that we should use git submodules for this ... I agree go mod vendoring should not be used for a project like BATS which is not go based

I think vendoring the external dependency is easier due being part of regular commits, we include the vendor directory in the project itself, as usual, and it makes everything simpler IMO. Git submodule is something I only have bad memories, and compared to the vendor option, it does require extra steps.

Git sub-modules are really the de-facto way to include external libraries like this in your repository.
Vendoring a project that has absolutely no Go code seems pretty sketchy.

Then, I think we can benefit from a more in-depth discussion about the git-submodules, @coreydaley. Seems a bit too much given we need a couple of assertions we can easily achieve with shell-script.

sounds like another topic for Monday / community meeting

pkg/shp/cmd/buildrun/create.go

otaviof · 2021-10-04T08:52:27Z

pkg/shp/flags/build.go

@@ -14,14 +15,14 @@ func BuildSpecFromFlags(flags *pflag.FlagSet) *buildv1alpha1.BuildSpec {
 	spec := &buildv1alpha1.BuildSpec{
 		Source: buildv1alpha1.Source{
 			Credentials: &corev1.LocalObjectReference{},
-			Revision:    pointer.String(""),
-			ContextDir:  pointer.String(""),
+			Revision:    pointer.StringPtr(""),


What's the reason for this change?

I think there was an issue when one of the dependencies was updated.

coreydaley · 2021-10-04T13:15:57Z

Some comments left on the pull-request, and I'm having issues with the Go modules changed. As per:

$ go mod vendor
go: github.com/shipwright-io/[email protected] (replaced by github.com/coreydaley/[email protected]): version "v0.0.0-20210927153915-3175d47a83e4" invalid: unknown revision 3175d47a83e4
go: downloading github.com/coreydaley/shipwright-io-build v0.0.0-20210927153915-3175d47a83e4
go: downloading github.com/spf13/cobra v1.2.1
go: downloading k8s.io/apimachinery v0.20.6
go: downloading k8s.io/client-go v0.20.2
go: downloading k8s.io/api v0.20.6
go: downloading github.com/onsi/gomega v1.10.3
go: downloading k8s.io/cli-runtime v0.20.2
go: downloading github.com/texttheater/golang-levenshtein/levenshtein v0.0.0-20200805054039-cae8b0eaed6c
go: downloading k8s.io/utils v0.0.0-20210111153108-fddb29f9d009
go: github.com/shipwright-io/[email protected] (replaced by github.com/coreydaley/[email protected]): version "v0.0.0-20210927153915-3175d47a83e4" invalid: unknown revision 3175d47a83e4

Same with go mod tidy, for the record.

Correct, since I am pulling in source from my own branch until my other api changes merge in shipwright-io/build.

gabemontero

so @coreydaley do you plan to add any valueFrom secret/configmap related options on the build/buildrun creates you added ... i.e. a --valueFromSecret= sort of thing

coreydaley · 2021-10-05T16:05:38Z

so @coreydaley do you plan to add any valueFrom secret/configmap related options on the build/buildrun creates you added ... i.e. a --valueFromSecret= sort of thing

No, @adambkaplan and I discussed it yesterday. I also don't think that oc ever added that capability (probably due to complexity)

gabemontero · 2021-10-07T18:57:20Z

/hold cancel

gabemontero · 2021-10-07T18:58:28Z

CI is green, including the e2e's

I'll

/approve

and will let @otaviof or @adambkaplan make the final pass for lgtm

openshift-ci · 2021-10-07T18:58:34Z

[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

go.mod

coreydaley · 2021-10-07T19:01:38Z

/hold

gabemontero · 2021-10-07T19:03:37Z

/hold

oops / apologies .... you want #51 to merge first ?

coreydaley · 2021-10-07T19:10:15Z

/hold cancel

gabemontero · 2021-10-07T19:33:27Z

ok, now @otaviof @adambkaplan with everything vendored in at the right levels and green tests, I'll wait on an lgtm and let one of you sign off since I already put the approve label previously

otaviof

On the final pass I found a couple of items, overall it looks good though!

docs/testing.md

hack/run-e2e.sh

test/e2e/e2e.bats

removing dependency for user to have to install BATS

coreydaley · 2021-10-08T12:56:54Z

@otaviof updates have been made per your suggestions. ptal

docs/testing.md

test/e2e/helpers.sh

https://issues.redhat.com/browse/BUILD-196

otaviof · 2021-10-08T15:52:11Z

/lgtm

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note labels Jun 23, 2021

openshift-ci bot requested review from adambkaplan and gabemontero June 23, 2021 19:59

openshift-ci bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 23, 2021

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 4, 2021

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 10, 2021

coreydaley added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 13, 2021

openshift-ci bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 23, 2021

coreydaley changed the title ~~[WIP] SHIP-0004: Build Environment Variables~~ SHIP-0004: Build Environment Variables Sep 27, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 27, 2021

coreydaley added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 27, 2021

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2021

gabemontero reviewed Sep 28, 2021

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 28, 2021

gabemontero mentioned this pull request Sep 30, 2021

Proxied Cluster Support shipwright-io/build#57

Open

otaviof requested changes Oct 4, 2021

View reviewed changes

gabemontero reviewed Oct 5, 2021

View reviewed changes

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 7, 2021

gabemontero reviewed Oct 7, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 7, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 7, 2021

otaviof self-requested a review October 8, 2021 02:38

otaviof requested changes Oct 8, 2021

View reviewed changes

docs/testing.md Show resolved Hide resolved

hack/run-e2e.sh Outdated Show resolved Hide resolved

test/e2e/e2e.bats Outdated Show resolved Hide resolved

otaviof reviewed Oct 8, 2021

View reviewed changes

test/e2e/e2e.bats Show resolved Hide resolved

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2021

coreydaley added 2 commits October 8, 2021 08:48

bump(*)

8d6dba5

adding BATS core and supporting libraries

bd63cf1

removing dependency for user to have to install BATS

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2021

otaviof reviewed Oct 8, 2021

View reviewed changes

docs/testing.md Outdated Show resolved Hide resolved

otaviof reviewed Oct 8, 2021

View reviewed changes

test/e2e/helpers.sh Show resolved Hide resolved

otaviof self-requested a review October 8, 2021 15:10

SHIP-0004: Build Environment Variables

0fef193

https://issues.redhat.com/browse/BUILD-196

otaviof approved these changes Oct 8, 2021

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 8, 2021

openshift-merge-robot merged commit 4831482 into shipwright-io:main Oct 8, 2021

SHIP-0004: Build Environment Variables #27

SHIP-0004: Build Environment Variables #27

Conversation

coreydaley commented Jun 23, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

coreydaley commented Sep 27, 2021

gabemontero commented Sep 28, 2021

Changes

Submitter Checklist

Release Notes

gabemontero commented Sep 28, 2021

coreydaley commented Sep 28, 2021

coreydaley commented Sep 28, 2021

gabemontero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabemontero commented Sep 28, 2021

gabemontero commented Sep 28, 2021

gabemontero commented Sep 29, 2021

otaviof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coreydaley commented Oct 4, 2021

gabemontero left a comment

Choose a reason for hiding this comment

coreydaley commented Oct 5, 2021

gabemontero commented Oct 7, 2021

gabemontero commented Oct 7, 2021

openshift-ci bot commented Oct 7, 2021

coreydaley commented Oct 7, 2021

gabemontero commented Oct 7, 2021

coreydaley commented Oct 7, 2021

gabemontero commented Oct 7, 2021

otaviof left a comment

Choose a reason for hiding this comment

coreydaley commented Oct 8, 2021 • edited Loading

otaviof commented Oct 8, 2021

coreydaley commented Jun 23, 2021 •

edited

Loading

coreydaley commented Oct 8, 2021 •

edited

Loading