Add fission/builder image

[erwinvaneyk]

This PR tackles a couple of issues with builder(mgr)

It...:

• ...fixes the issue where the environment-scoped buildcmd was not used.
• ...allows files to be passed to the builder
• ...adds a fission/builder docker image avoiding the need to compile fission to get the binary.
• ...fixes the issue where the builder did not log the stderr

As a flyby, I updated the deploy script to correspond with the one in fission workflows

cc @life1347

---

This change is 

[soamvasani]

@life1347 please take a look

[erwinvaneyk]

@soamvasani test fails because of missing fission/builder image. If you agree with the approach taken here, we can push the image before more merging and rerun the tests to make it all green

[soamvasani]

Hrm, what if a builder wants to do something like FROM golang? Maybe we can add the builder binary to the image using a shared volume instead?

[erwinvaneyk]

That would also be a solution. The old approach is still possible allowing for build environments to extend other images. Though the image is more of a convenience. It is just a pain to setup up cross-project build chains to get the binary.

[soamvasani]

Yes I agree, the cross project thing is annoying, and the current approach (in the master branch, not this PR) is also kinda weird.

Okay, so let's go with this approach of having a builder image, given that it isn't stopping us from the alternatives.

I think there's a way to test it by removing the hard-coding of the fission/builder image in the Dockerfile. That way the CI actually tests the builder, e.g. if you have builder changes you want those tested, not the last release. You can parameterize this by using a docker "ARG" for the image name and tag: https://docs.docker.com/engine/reference/builder/#understand-how-arg-and-from-interact . So if you do that you also won't need the image to be pushed for the tests on this PR to pass.

[erwinvaneyk]

@soamvasani thanks for the tip to use build args. I did have to add a docker update step to travis, because the default docker version of travis does not support build args yet.

[soamvasani]

Reviewed 8 of 9 files at r1, 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

builder/builder.go, line 184 at r2 (raw file):

	if err := scanner.Err(); err != nil {
		fmt.Println(err)

log some sort of message, not just the error; you could log the same message that's being returned.

---

builder/builder.go, line 190 at r2 (raw file):

	err = cmd.Wait()
	if err != nil {
		fmt.Println(err)

same comment, log a message + the error

---

buildermgr/envwatcher.go, line 452 at r2 (raw file):

							Name:                   "builder",
							Image:                  env.Spec.Builder.Image,
							ImagePullPolicy:        apiv1.PullIfNotPresent,

Doesn't this break the integration tests? I think you'll need an env var that can be set on the deployment, just like we do with fetcher in the tests.

---

buildermgr/pkgwatcher.go, line 85 at r2 (raw file):

			rv = pkg.Metadata.ResourceVersion

			// only build packages in pending state

please avoid unnecessary changes

---

hack/deploy.sh, line 1 at r2 (raw file):

#!/usr/bin/env bash

Can you create a separate PR for this file? I'm not sure how it relates to this PR?

---

Comments from Reviewable

[erwinvaneyk]

Review status: 9 of 13 files reviewed at latest revision, 5 unresolved discussions.

---

buildermgr/envwatcher.go, line 452 at r2 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Doesn't this break the integration tests? I think you'll need an env var that can be set on the deployment, just like we do with fetcher in the tests.

An env var or we could specify it in the env.Spec.Builder?

---

buildermgr/pkgwatcher.go, line 85 at r2 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

please avoid unnecessary changes

Will try to avoid it in future prs 👍

---

hack/deploy.sh, line 1 at r2 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Can you create a separate PR for this file? I'm not sure how it relates to this PR?

It indeed is a flyby as specified in the main post. Will pull it out of this PR.

---

Comments from Reviewable

[erwinvaneyk]

Review status: 9 of 13 files reviewed at latest revision, 5 unresolved discussions.

---

builder/builder.go, line 184 at r2 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

log some sort of message, not just the error; you could log the same message that's being returned.

Done.

---

builder/builder.go, line 190 at r2 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

same comment, log a message + the error

Done.

---

Comments from Reviewable

[erwinvaneyk]

Review status: 9 of 13 files reviewed at latest revision, 5 unresolved discussions.

---

hack/deploy.sh, line 1 at r2 (raw file):

Previously, erwinvaneyk (Erwin van Eyk) wrote…

It indeed is a flyby as specified in the main post. Will pull it out of this PR.

Done.

---

Comments from Reviewable

[soamvasani]

Reviewed 1 of 9 files at r1, 2 of 4 files at r3, 1 of 1 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

builder/builder.go, line 184 at r2 (raw file):

Previously, erwinvaneyk (Erwin van Eyk) wrote…

Done.

Nitpick, but the var name msg suggests a string, while this is actually an error object.

---

builder/cmd/builder, line 0 at r5 (raw file):
unintentional checkin?

---

buildermgr/envwatcher.go, line 452 at r2 (raw file):

Previously, erwinvaneyk (Erwin van Eyk) wrote…

An env var or we could specify it in the env.Spec.Builder?

Either will work but I'd like to keep things consistent with other places in the project, such as fetcher. I suggest using an env var right now and adding it to the spec in a separate change.

---

buildermgr/pkgwatcher.go, line 85 at r2 (raw file):

Previously, erwinvaneyk (Erwin van Eyk) wrote…

Will try to avoid it in future prs 👍

Thanks. But please remove this one too.

---

charts/fission-all-0.4.0rc.tgz, line 0 at r4 (raw file):
Unintentionally checked in?

---

Comments from Reviewable

[erwinvaneyk]

Review status: 8 of 9 files reviewed at latest revision, 6 unresolved discussions.

---

buildermgr/envwatcher.go, line 452 at r2 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Either will work but I'd like to keep things consistent with other places in the project, such as fetcher. I suggest using an env var right now and adding it to the spec in a separate change.

For the sake of getting this PR merged soonish, I want to revert it back to pullAlways, and defer the pull policy change to a later PR. Agreed?

---

Comments from Reviewable

[soamvasani]

LGTM

---

Reviewed 5 of 5 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable