feat: s2i builder with preliminary node support #923

lkingland · 2022-03-30T02:41:31Z

Changes

🎁 Source-to-Image Builder implementation which can be specified with --builder=s2i
🎁 Adds preliminary support for building Node.js Functions with the s2i builder.

/kind enhancement

First step towards accomplishing #908

knative-prow · 2022-03-30T02:41:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lkingland

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~~ [lkingland]

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

codecov · 2022-03-30T02:58:01Z

Codecov Report

Merging #923 (ed0f767) into main (739cded) will increase coverage by 2.22%.
The diff coverage is 44.30%.

@@            Coverage Diff             @@
##             main     #923      +/-   ##
==========================================
+ Coverage   43.85%   46.07%   +2.22%     
==========================================
  Files          55       56       +1     
  Lines        5163     7395    +2232     
==========================================
+ Hits         2264     3407    +1143     
- Misses       2576     3660    +1084     
- Partials      323      328       +5

Impacted Files	Coverage Δ
client.go	`61.94% <0.00%> (+2.01%)`	⬆️
cmd/completion_util.go	`0.00% <0.00%> (ø)`
docker/docker_client.go	`82.52% <ø> (+0.57%)`	⬆️
s2i/builder.go	`32.69% <32.69%> (ø)`
cmd/build.go	`60.49% <70.83%> (+0.86%)`	⬆️
cmd/root.go	`72.28% <100.00%> (+1.66%)`	⬆️
cmd/list.go	`56.25% <0.00%> (-3.40%)`	⬇️
cmd/info.go	`24.77% <0.00%> (-2.38%)`	⬇️
function_buildtype.go	`92.30% <0.00%> (-2.14%)`	⬇️
cmd/run.go	`71.42% <0.00%> (-1.30%)`	⬇️
... and 51 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a4cebb...ed0f767. Read the comment docs.

matejvasek · 2022-04-04T12:47:06Z

@lkingland why the changes to the node/go templates?

lkingland · 2022-04-04T12:50:33Z

@lkingland why the changes to the node/go templates?

Builders are no longer defined in a template's manifest, because a Function can be built with either a Buildpack or an S2I builder, and thus the default is dependant on the builder chosen at build time.

These defaults should have been in the code from the beginning, and this is the forcing function.

matejvasek · 2022-04-04T12:53:11Z

These defaults should have been in the code from the beginning, and this is the forcing function.

I think we had defaults in code and then removed it.

matejvasek · 2022-04-04T12:56:46Z

But the defaults are only for embedded templates. We will have to update manifest.yaml structure latter before 1.0.

matejvasek · 2022-04-04T18:14:12Z

@lkingland s2i build is not working with podman.
Probably ok for now, but we should put it into documentation.

openshift/source-to-image#1082

matejvasek · 2022-04-04T19:20:25Z

containers/podman#13770

…builder

s2i/builder.go

lance

Looks like a great first step into s2i builds. There are some things I think we'll want to consider in the long run, such as how we deal with chained builds (important to avoid having 500MB application images), and perhaps other things of a similar nature. A few questions within, but overall lgtm.

lance · 2022-04-04T14:06:55Z

buildpacks/builder.go

+// DefaultBuilderImages for Pack builders indexed by Runtime Language
+var DefaultBuilderImages = map[string]string{
+	"node": "gcr.io/paketo-buildpacks/builder:base",
+	"go":   "gcr.io/paketo-buildpacks/builder:base",


This won't really work for Go functions though, will it? They need the scaffolding that is in the buildpack as well, right?

It is my understanding that the base is the same, but the buildpack builder will still apply any buildpacks defined, which include ghcr.io/boson-project/go-function-buildpack:tip, which has the scaffolding. The S2I builder, when updated to support Go, will of course ignore that and use its own method, which I presume will be a single builder image that includes everything required.

It is my understanding that the base is the same, but the buildpack builder will still apply any buildpacks defined, which include ghcr.io/boson-project/go-function-buildpack:tip, which has the scaffolding.

Yes, this is the case, if it is explicitly provided, for example in a func.yaml file in the buildpacks field. Is that what you are expecting? Without that, the scaffold isn't there.

lance · 2022-04-04T14:14:01Z

cmd/build.go

 	}

-	cmd.Flags().StringP("builder", "b", "", "Buildpack builder, either an as a an image name or a mapping name.\nSpecified value is stored in func.yaml for subsequent builds.")
+	cmd.Flags().StringP("builder", "b", "pack", "builder to use when creating the underlying image. Currently supported builders are 'pack' and 's2i'.")
+	cmd.Flags().StringP("builder-image", "", "", "builder image, either an as a an image name or a mapping name.\nSpecified value is stored in func.yaml for subsequent builds. ($FUNC_BUILDER_IMAGE)")


It strikes me as clumsy that there is both a builder and build-image flag. I wonder if it should rather be something like strategy and build-image (or build), where strategy is a fixed set (buildpack, s2i, etc) and build-image or build is an image address.

I agree. We should try to consolidate this a little bit. I wouldn't block this PR on this though. But it is something we should try to tackle as soon as possilbe. To provide consistency and more clarity for users.

In order to not block this PR, I have opened a new issue to discuss better flag naming: #942

go.mod

s2i/builder.go

lance · 2022-04-04T20:28:03Z

s2i/builder.go

+		return errors.New("Unable to build via the s2i builder.")
+	}
+
+	builder, _, err := strategies.Strategy(client, cfg, build.Overrides{})


Am I correct to understand this is the "docker" build strategy? One of the other build strategies that s2i uses is "binary" which we may consider leveraging for push-to-cluster-my-function-and-build-it-there, since that's exactly what it does.

Yes, it is my understanding this is the Docker method. I am not familiar with the binary strategy, but I will look into it.

lance · 2022-04-04T20:31:53Z

test/_e2e/cmd_build_test.go

+	)
+	defer cleanup()
+
+	run(t, bin, prefix, "create", "-v", "--language=node", cwd)


Did you test with typescript? I don't know if s2i can seamlessly do that or not, but it sure would be nice

I have not run a typescript test yet, but I am hopeful as well.

matejvasek · 2022-04-04T20:51:59Z

/lgtm

matejvasek · 2022-04-04T20:52:05Z

/hold
for others

zroubalik

/lgtm

matejvasek · 2022-04-05T12:33:58Z

/lgtm

knative-prow bot added kind/enhancement size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 30, 2022

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 30, 2022

lkingland mentioned this pull request Mar 30, 2022

feat: builder flag with s2i option #842

Closed

lkingland marked this pull request as draft March 30, 2022 02:45

knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 30, 2022

lkingland force-pushed the lkingland/s2i-node-builder branch from db026c6 to ff8ac0e Compare March 30, 2022 02:51

lkingland force-pushed the lkingland/s2i-node-builder branch from ea94b47 to ef8cf90 Compare March 31, 2022 13:36

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 31, 2022

lkingland added 13 commits March 31, 2022 22:47

fix: stuck build ticker

a6cde9e

feat: s2i builder prototype

bbddc0a

default builders

d199769

use s2i fork with updated docker

baf9e04

in-code builder defaults

e9127a7

s2i builder verbosity constructor arg

9d962dd

typed errors

5db172d

typed error tests

02bda79

remove unneeded env code

356dcfa

s2i build e2e test

836516b

e2e tests

1c826e8

update licenses

917eb48

cleanup

bd3503a

lkingland force-pushed the lkingland/s2i-node-builder branch from da706bd to 9d70e2e Compare March 31, 2022 13:50

codegen debug

f3a9a55

lkingland force-pushed the lkingland/s2i-node-builder branch from 9d70e2e to f3a9a55 Compare March 31, 2022 13:56

Merge branch 'main' into lkingland/s2i-node-builder

2e7aa5a

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 31, 2022

update licenses

e5a89a5

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 2, 2022

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 4, 2022

Merge remote-tracking branch 'upstream/main' into lkingland/s2i-node-…

4cb60fa

…builder

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 4, 2022

matejvasek reviewed Apr 4, 2022

View reviewed changes

s2i/builder.go Outdated Show resolved Hide resolved

matejvasek reviewed Apr 4, 2022

View reviewed changes

s2i/builder.go Outdated Show resolved Hide resolved

lance reviewed Apr 4, 2022

View reviewed changes

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 4, 2022

zroubalik reviewed Apr 5, 2022

View reviewed changes

knative-prow bot assigned zroubalik Apr 5, 2022

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 5, 2022

lkingland mentioned this pull request Apr 5, 2022

Improve Builder Flag Names #942

Closed

use GetDefaultDockerConfig for s2i config

95da554

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 5, 2022

docker.NewClient docs

ed0f767

knative-prow bot assigned matejvasek Apr 5, 2022

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 5, 2022

lkingland removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 5, 2022

knative-prow bot merged commit a91bcc5 into knative:main Apr 5, 2022

lance mentioned this pull request Apr 6, 2022

Add support for s2i builds #908

Closed

lkingland deleted the lkingland/s2i-node-builder branch September 20, 2022 01:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: s2i builder with preliminary node support #923

feat: s2i builder with preliminary node support #923

lkingland commented Mar 30, 2022 •

edited

Loading

knative-prow bot commented Mar 30, 2022

codecov bot commented Mar 30, 2022 •

edited

Loading

matejvasek commented Apr 4, 2022

lkingland commented Apr 4, 2022

matejvasek commented Apr 4, 2022

matejvasek commented Apr 4, 2022 •

edited

Loading

matejvasek commented Apr 4, 2022

matejvasek commented Apr 4, 2022

lance left a comment

lance Apr 4, 2022

lkingland Apr 5, 2022 •

edited

Loading

lance Apr 5, 2022

lance Apr 4, 2022

zroubalik Apr 5, 2022

lkingland Apr 5, 2022

lance Apr 4, 2022

lkingland Apr 5, 2022

lance Apr 4, 2022

lkingland Apr 5, 2022

matejvasek commented Apr 4, 2022 •

edited

Loading

matejvasek commented Apr 4, 2022 •

edited

Loading

zroubalik left a comment

matejvasek commented Apr 5, 2022

feat: s2i builder with preliminary node support #923

feat: s2i builder with preliminary node support #923

Conversation

lkingland commented Mar 30, 2022 • edited Loading

Changes

knative-prow bot commented Mar 30, 2022

codecov bot commented Mar 30, 2022 • edited Loading

Codecov Report

matejvasek commented Apr 4, 2022

lkingland commented Apr 4, 2022

matejvasek commented Apr 4, 2022

matejvasek commented Apr 4, 2022 • edited Loading

matejvasek commented Apr 4, 2022

matejvasek commented Apr 4, 2022

lance left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkingland Apr 5, 2022 • edited Loading

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

matejvasek commented Apr 4, 2022 • edited Loading

matejvasek commented Apr 4, 2022 • edited Loading

zroubalik left a comment

Choose a reason for hiding this comment

matejvasek commented Apr 5, 2022

lkingland commented Mar 30, 2022 •

edited

Loading

codecov bot commented Mar 30, 2022 •

edited

Loading

matejvasek commented Apr 4, 2022 •

edited

Loading

lkingland Apr 5, 2022 •

edited

Loading

matejvasek commented Apr 4, 2022 •

edited

Loading

matejvasek commented Apr 4, 2022 •

edited

Loading