allowing on cluster build for go runtime #1445

Shashankft9 · 2022-11-23T12:02:31Z

Changes

🧹 allow on cluster build to happen for go runtime, using boson go builder
🧹 updating the documention for on cluster build to fix the existing steps and what has to be added further for go runtime

/kind cleanup

Fixes #809

Release Note

Docs

linux-foundation-easycla · 2022-11-23T12:02:35Z

✅ login: Shashankft9 / name: Shashank Sharma (ff1e364)
❌ The email address for the commit (57a4b6a, 63a2a55) is not linked to the GitHub account, preventing the EasyCLA check. Consult this Help Article and GitHub Help to resolve. (To view the commit's email address, add .patch at the end of this PR page's URL.) For further assistance with EasyCLA, please submit a support request ticket.

codecov · 2022-11-23T12:14:00Z

Codecov Report

Patch coverage: 53.57% and project coverage change: +0.59% 🎉

Comparison is base (5948cf4) 62.02% compared to head (7a4b2ff) 62.61%.
Report is 14 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1445      +/-   ##
==========================================
+ Coverage   62.02%   62.61%   +0.59%     
==========================================
  Files         107      106       -1     
  Lines       13733    13817      +84     
==========================================
+ Hits         8518     8652     +134     
+ Misses       4375     4278      -97     
- Partials      840      887      +47

Flag	Coverage Δ
e2e-test	`35.49% <0.00%> (-0.11%)`	⬇️
e2e-test-oncluster	`30.60% <17.85%> (-0.04%)`	⬇️
e2e-test-oncluster-runtime	`25.65% <21.42%> (?)`
e2e-test-runtime-go	`25.92% <0.00%> (?)`
e2e-test-runtime-node	`26.92% <0.00%> (?)`
e2e-test-runtime-python	`26.92% <0.00%> (?)`
e2e-test-runtime-quarkus	`27.04% <0.00%> (?)`
e2e-test-runtime-springboot	`26.06% <0.00%> (?)`
e2e-test-runtime-typescript	`27.04% <0.00%> (?)`
integration-tests	`51.36% <25.00%> (+1.98%)`	⬆️
unit-tests-macos-latest	`49.17% <39.28%> (-0.06%)`	⬇️
unit-tests-ubuntu-latest	`?`
unit-tests-windows-latest	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
pkg/pipelines/tekton/pipelines_pac_provider.go	`42.77% <33.33%> (+37.18%)`	⬆️
pkg/pipelines/tekton/pipelines_provider.go	`57.18% <33.33%> (+1.87%)`	⬆️
pkg/pipelines/tekton/validate.go	`77.77% <68.75%> (-12.23%)`	⬇️

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

matejvasek · 2022-11-23T22:49:00Z

@Shashankft9 does this depend on #984 ?

Shashankft9 · 2022-11-24T02:41:13Z

thanks @matejvasek for linking that issue, seems like a better solution specially since i got to know yesterday that this boson go builder is not maintained anymore,
I wonder, if this can fly as a temporary solution though, since not much has changed programmatically?

matejvasek · 2022-11-30T12:21:31Z

pipelines/tekton/validate.go

@@ -30,7 +30,7 @@ func validatePipeline(f fn.Function) error {
 			return ErrRuntimeRequired
 		}

-		if f.Runtime == "go" || f.Runtime == "rust" {
+		if f.Runtime == "rust" {


Maybe we could remove this if completely since following if check if there are additional buildpacks.

I think it might be better to keep it here, to explicitly tell user about not supported runtime.

But the issue with Go and Rust is that they use custom buildpacks.
That problem is detected in following if.

What this if does is it prevents users from building Go and Rust apps even if user provides custom builder that would work all right.

@zroubalik @Shashankft9 what about just printing warning here "Go/Rust default builder do not support in cluster build" instead of returning an error? Could we accept that?

@zroubalik @Shashankft9 what about just printing warning here "Go/Rust default builder do not support in cluster build" instead of returning an error? Could we accept that?

@Shashankft9 Can you add a simple warning here that Go and Rust default builders do not support in-cluster builds?

matejvasek · 2022-11-30T12:24:45Z

docs/building-functions/on_cluster_build.md

+  # builderImages:                                    # optional, needed only if the runtime is golang 
+  #   pack: ghcr.io/boson-project/go-function-builder:tip
+  buildpacks: []
+  builder: ""


Builder should be "pack".

but empty string probably would work too

If these two items are intended as hints, commenting them would probably be best.

The idea here is that the code has static defaults. Anything placed in the config file is specifically requesting that exact value, so while the first value, []buildpacks is innocuous, the second, builder="" is, if literally translated, requesting the system to build without a builder. In this case we do set the default if the value comes in empty like this, because building without a builder is nonsensical, but I presume this exists to give a hint to the user how to choose a builder and buildpacks, so a commented entry is the right tool for the job.

matejvasek · 2022-11-30T12:28:02Z

@Shashankft9 unittests are failing.

matejvasek · 2022-11-30T12:35:57Z

@zroubalik PTAL

matejvasek · 2022-12-01T22:24:59Z

@Shashankft9 Go build issue might be fixed by #1451 .

zroubalik

we should wait with this until go is supported with on cluster builds.

Or am I missing anything?

matejvasek · 2022-12-14T15:13:18Z

we should wait with this until go is supported with on cluster builds.

Or am I missing anything?

The thing is we are disabling Go build no matter what -- even if user provided custom builder that can do build even now.

github-actions · 2023-03-15T01:28:42Z

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.

lkingland · 2023-03-20T12:35:48Z

FYI I am using this as an important reference/ starting point for the solution discussed in #1589
Not sure if that means we will include these commits directly ¯_(ツ)_/¯ but perhaps best to keep it open?

Shashankft9 · 2023-05-11T13:48:08Z

hey, a gentle ping:

I did an update on the go builder here: https://github.com/boson-project/packs with the latest pack full cnb and go-dist BP. And then the same changes in this PR are making my on cluster build work for go functions.

I did the update of the builder because it was failing for my functions which were using latest k8s imports.

I am aware that the plan for goruntime's oncluster build is much broader, but I am just checking back again if there is scope to still allow a customer builder like this one, following @matejvasek comment

The thing is we are disabling Go build no matter what -- even if user provided custom builder that can do build even now.

lkingland

This looks mostly good to me, but we could probably use a warning, as suggested, and of course a rebase to resolve the conflicts.

lkingland · 2023-06-19T06:39:10Z

docs/building-functions/on_cluster_build.md

+  # builderImages:                                    # optional, needed only if the runtime is golang 
+  #   pack: ghcr.io/boson-project/go-function-builder:tip
+  buildpacks: []
+  builder: ""


If these two items are intended as hints, commenting them would probably be best.

The idea here is that the code has static defaults. Anything placed in the config file is specifically requesting that exact value, so while the first value, []buildpacks is innocuous, the second, builder="" is, if literally translated, requesting the system to build without a builder. In this case we do set the default if the value comes in empty like this, because building without a builder is nonsensical, but I presume this exists to give a hint to the user how to choose a builder and buildpacks, so a commented entry is the right tool for the job.

lkingland · 2023-06-19T06:47:42Z

pipelines/tekton/validate.go

@@ -30,7 +30,7 @@ func validatePipeline(f fn.Function) error {
 			return ErrRuntimeRequired
 		}

-		if f.Runtime == "go" || f.Runtime == "rust" {
+		if f.Runtime == "rust" {


@zroubalik @Shashankft9 what about just printing warning here "Go/Rust default builder do not support in cluster build" instead of returning an error? Could we accept that?

@Shashankft9 Can you add a simple warning here that Go and Rust default builders do not support in-cluster builds?

Shashankft9 · 2023-07-20T14:16:10Z

just signed the CLA again.

@lkingland and others - hey, apologies for the delay on this, got caught up in other work. This is just a quick fix, let me know if it looks alright, I'll add some more tests

linux-foundation-easycla · 2023-07-21T09:47:08Z

✅ login: Shashankft9 / name: Shashank Sharma (ff1e364)
❌ The email address for the commit (57a4b6a, 63a2a55, 360cfef) is not linked to the GitHub account, preventing the EasyCLA check. Consult this Help Article and GitHub Help to resolve. (To view the commit's email address, add .patch at the end of this PR page's URL.) For further assistance with EasyCLA, please submit a support request ticket.

Signed-off-by: Matej Vasek <mvasek@redhat.com>

matejvasek · 2023-07-25T21:27:46Z

PTAL @lance

matejvasek · 2023-07-25T21:30:39Z

@Shashankft9 I pushed on fixup.

lance · 2023-07-26T00:37:31Z

I think this is the symlink issue on windows that we have been dealing with lately. @matejvasek please lmk if I am mistaken.

test build error rename C:\Users\RUNNER~1\AppData\Local\Temp\TestBuilder_Concurrency2845656642\001\.func\builds\by-hash\70edc4a387fa606bf967e52bea2cc6ce6762ce9eeda58e617cfbf99a3514fd73\config.json C:\Users\RUNNER~1\AppData\Local\Temp\TestBuilder_Concurrency2845656642\001\.func\builds\by-hash\70edc4a387fa606bf967e52bea2cc6ce6762ce9eeda58e617cfbf99a3514fd73\oci\blobs\sha256\8a7469839f911077ecb8405595926905343df847dfed79d760a130c1953d91ee: The system cannot find the path specified.	knative.dev/func/pkg/oci	coverage: 71.6% of statements

/override "Unit Test (1.20.2, 17, windows-latest)"

knative-prow · 2023-07-26T00:37:34Z

@lance: Overrode contexts on behalf of lance: Unit Test (1.20.2, 17, windows-latest)

In response to this:

I think this is the symlink issue on windows that we have been dealing with lately. @matejvasek please lmk if I am mistaken.

test build error rename C:\Users\RUNNER~1\AppData\Local\Temp\TestBuilder_Concurrency2845656642\001\.func\builds\by-hash\70edc4a387fa606bf967e52bea2cc6ce6762ce9eeda58e617cfbf99a3514fd73\config.json C:\Users\RUNNER~1\AppData\Local\Temp\TestBuilder_Concurrency2845656642\001\.func\builds\by-hash\70edc4a387fa606bf967e52bea2cc6ce6762ce9eeda58e617cfbf99a3514fd73\oci\blobs\sha256\8a7469839f911077ecb8405595926905343df847dfed79d760a130c1953d91ee: The system cannot find the path specified.	knative.dev/func/pkg/oci	coverage: 71.6% of statements

/override "Unit Test (1.20.2, 17, windows-latest)"

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

matejvasek · 2023-07-28T11:51:14Z

/approve
/lgtm

knative-prow · 2023-07-28T11:51:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matejvasek, Shashankft9

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

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

Shashankft9 · 2023-07-28T12:51:54Z

@matejvasek would be amazing if you can take a look at this too: boson-project/packs#10

matejvasek · 2023-07-28T20:03:45Z

@Shashankft9 does this work now? I am afraid #1774 is breaking it.

matejvasek · 2023-07-28T20:24:03Z

Shashankft9 does this work now? I am afraid #1774 is breaking it.

Well it is probably not breaking on-cluster-build, but it breaks local build.

* allowing on cluster build for go runtime * warning message added for go and rust builder * gofmt * fixups Signed-off-by: Matej Vasek <mvasek@redhat.com> --------- Signed-off-by: Matej Vasek <mvasek@redhat.com> Co-authored-by: Matej Vasek <mvasek@redhat.com>

* chore: use tkn tasks from PR branch in CI (#1914) Signed-off-by: Matej Vasek <mvasek@redhat.com> * chore: released binaries refer correct task yamls (#1916) Signed-off-by: Matej Vasek <mvasek@redhat.com> * chore: refer correct tkn yaml in prow test (#1918) Without this change prow test will refer tkn yamls from the main branch not from the PR head branch. Signed-off-by: Matej Vasek <mvasek@redhat.com> * chore: update release generation (#1924) * Minimize release binary size. * Release latest version of buildpack tekton task. Signed-off-by: Matej Vasek <mvasek@redhat.com> * test: update github ref used on e2e oncluster tests (#1917) * test: Split of GH oncluster tests by builder. Added FUNC_BUILDER env var for e2e oncluter tests (#1963) * Use our own s2i image (#1971) Our image is much more recent and it is multiarch. Signed-off-by: Matej Vasek <mvasek@redhat.com> * chore: update buildah image ref (#1960) Signed-off-by: Matej Vasek <mvasek@redhat.com> * chore: update CA certs (#1944) Signed-off-by: Matej Vasek <mvasek@redhat.com> * chore: update Quarkus platform version to 3.4.1 (#1989) Co-authored-by: Knative Automation <automation@knative.team> Signed-off-by: Matej Vasek <mvasek@redhat.com> * chore: update mvn wrapper in Quarkus template (#1987) Signed-off-by: Matej Vasek <mvasek@redhat.com> * chore: use ./mvnw not mvn in tests (#1988) Signed-off-by: Matej Vasek <mvasek@redhat.com> * chore: update Springboot platform version Signed-off-by: Matej Vasek <mvasek@redhat.com> * fix: docker registry/repository parsing (#1929) * fix: docker registry/repository parsing Use go-containerregistry to do parsing. Signed-off-by: Matej Vasek <mvasek@redhat.com> * fix: use kebab-case instead of camelCase Signed-off-by: Matej Vasek <mvasek@redhat.com> * fix: use kebab-case instead of camelCase Signed-off-by: Matej Vasek <mvasek@redhat.com> --------- Signed-off-by: Matej Vasek <mvasek@redhat.com> * Fix failing concurrent test on Windows (#1890) * src: better debugging Signed-off-by: Matej Vasek <mvasek@redhat.com> * fix: wait for both builds Signed-off-by: Matej Vasek <mvasek@redhat.com> * fixup Signed-off-by: Matej Vasek <mvasek@redhat.com> * fixup Signed-off-by: Matej Vasek <mvasek@redhat.com> * fixup Signed-off-by: Matej Vasek <mvasek@redhat.com> * fix: detection of process liveness on Windows Signed-off-by: Matej Vasek <mvasek@redhat.com> * fix: make symlink relative Signed-off-by: Matej Vasek <mvasek@redhat.com> * fixup: cleanup Signed-off-by: Matej Vasek <mvasek@redhat.com> --------- Signed-off-by: Matej Vasek <mvasek@redhat.com> * allowing on cluster build for go runtime (#1445) * allowing on cluster build for go runtime * warning message added for go and rust builder * gofmt * fixups Signed-off-by: Matej Vasek <mvasek@redhat.com> --------- Signed-off-by: Matej Vasek <mvasek@redhat.com> Co-authored-by: Matej Vasek <mvasek@redhat.com> * Use custom jammy paketo builder (#1911) * chore: use custom jammy paketo builder Use our own modified jammy builder with additional buildpacks for GoFunc and Rust. This enables on cluster build for Go and Rust functions. Where possible (Go, Java) we use "tiny" variant, other runtimes use "base" variant. The updated task is new file instead of modifying existing task this is done for sake of keeping compatiblility. Signed-off-by: Matej Vasek <mvasek@redhat.com> * fixup: remove unnecessary code per review request Signed-off-by: Matej Vasek <mvasek@redhat.com> * fixup Signed-off-by: Matej Vasek <mvasek@redhat.com> * fixup: podman test refers correct tkn task yamls Signed-off-by: Matej Vasek <mvasek@redhat.com> --------- Signed-off-by: Matej Vasek <mvasek@redhat.com> * doc: Go and Rust on cluster build is supported (#1923) * doc: Go and Rust on cluster build is supported Signed-off-by: Matej Vasek <mvasek@redhat.com> * doc: build envvars Signed-off-by: Matej Vasek <mvasek@redhat.com> --------- Signed-off-by: Matej Vasek <mvasek@redhat.com> * fix: direct upload ppc64le, x390x (#1958) Signed-off-by: Matej Vasek <mvasek@redhat.com> * fix: report correct error when task doesn't exist (#1915) Signed-off-by: Matej Vasek <mvasek@redhat.com> * feat: tekton task urls in the env sub-cmd output (#1925) Signed-off-by: Matej Vasek <mvasek@redhat.com> --------- Signed-off-by: Matej Vasek <mvasek@redhat.com> Co-authored-by: Jefferson Ramos <jeramos@redhat.com> Co-authored-by: Knative Automation <automation@knative.team> Co-authored-by: Shashank Sharma <48708039+Shashankft9@users.noreply.github.com>

allowing on cluster build for go runtime

ff1e364

knative-prow bot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Nov 23, 2022

knative-prow bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 23, 2022

matejvasek reviewed Nov 30, 2022

View reviewed changes

matejvasek requested review from zroubalik and lkingland November 30, 2022 12:28

lance mentioned this pull request Dec 1, 2022

Add go function invocation runtime as a package #1451

Closed

3 tasks

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

zroubalik reviewed Dec 14, 2022

View reviewed changes

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 15, 2023

github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 21, 2023

lkingland reviewed Jun 19, 2023

View reviewed changes

Shashankft9 mentioned this pull request Jul 14, 2023

builder updated boson-project/packs#10

Merged

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

knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 20, 2023

Shashankft9 added 3 commits July 21, 2023 15:23

rebase

6776ab0

warning message added for go and rust builder

9939828

gofmt

2469823

Shashankft9 force-pushed the go-on-cluster branch from 360cfef to 2469823 Compare July 21, 2023 09:56

fixups

7a4b2ff

Signed-off-by: Matej Vasek <mvasek@redhat.com>

knative-prow bot assigned matejvasek Jul 28, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 28, 2023

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2023

knative-prow bot merged commit 2463202 into knative:main Jul 28, 2023
40 of 41 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allowing on cluster build for go runtime #1445

allowing on cluster build for go runtime #1445

Shashankft9 commented Nov 23, 2022

linux-foundation-easycla bot commented Nov 23, 2022 •

edited

Loading

codecov bot commented Nov 23, 2022 •

edited

Loading

matejvasek commented Nov 23, 2022

Shashankft9 commented Nov 24, 2022

matejvasek Nov 30, 2022

zroubalik Dec 14, 2022

matejvasek Dec 14, 2022

matejvasek May 24, 2023

lkingland Jun 19, 2023

matejvasek Nov 30, 2022

matejvasek Nov 30, 2022

lkingland Jun 19, 2023

matejvasek commented Nov 30, 2022

matejvasek commented Nov 30, 2022

matejvasek commented Dec 1, 2022

zroubalik left a comment

matejvasek commented Dec 14, 2022 •

edited

Loading

github-actions bot commented Mar 15, 2023

lkingland commented Mar 20, 2023 •

edited

Loading

Shashankft9 commented May 11, 2023

lkingland left a comment

lkingland Jun 19, 2023

lkingland Jun 19, 2023

Shashankft9 commented Jul 20, 2023

linux-foundation-easycla bot commented Jul 21, 2023

matejvasek commented Jul 25, 2023

matejvasek commented Jul 25, 2023

lance commented Jul 26, 2023

knative-prow bot commented Jul 26, 2023

matejvasek commented Jul 28, 2023

knative-prow bot commented Jul 28, 2023

Shashankft9 commented Jul 28, 2023

matejvasek commented Jul 28, 2023

matejvasek commented Jul 28, 2023 •

edited

Loading

allowing on cluster build for go runtime #1445

allowing on cluster build for go runtime #1445

Conversation

Shashankft9 commented Nov 23, 2022

Changes

linux-foundation-easycla bot commented Nov 23, 2022 • edited Loading

codecov bot commented Nov 23, 2022 • edited Loading

Codecov Report

matejvasek commented Nov 23, 2022

Shashankft9 commented Nov 24, 2022

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 Nov 30, 2022

matejvasek commented Nov 30, 2022

matejvasek commented Dec 1, 2022

zroubalik left a comment

Choose a reason for hiding this comment

matejvasek commented Dec 14, 2022 • edited Loading

github-actions bot commented Mar 15, 2023

lkingland commented Mar 20, 2023 • edited Loading

Shashankft9 commented May 11, 2023

lkingland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shashankft9 commented Jul 20, 2023

linux-foundation-easycla bot commented Jul 21, 2023

matejvasek commented Jul 25, 2023

matejvasek commented Jul 25, 2023

lance commented Jul 26, 2023

knative-prow bot commented Jul 26, 2023

matejvasek commented Jul 28, 2023

knative-prow bot commented Jul 28, 2023

Shashankft9 commented Jul 28, 2023

matejvasek commented Jul 28, 2023

matejvasek commented Jul 28, 2023 • edited Loading

linux-foundation-easycla bot commented Nov 23, 2022 •

edited

Loading

codecov bot commented Nov 23, 2022 •

edited

Loading

matejvasek commented Dec 14, 2022 •

edited

Loading

lkingland commented Mar 20, 2023 •

edited

Loading

matejvasek commented Jul 28, 2023 •

edited

Loading