Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allowing on cluster build for go runtime #1445

Merged
merged 5 commits into from
Jul 28, 2023

Conversation

Shashankft9
Copy link
Member

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


@knative-prow 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
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 23, 2022

CLA Missing ID CLA Not Signed

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

codecov bot commented Nov 23, 2022

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
Copy link
Contributor

@Shashankft9 does this depend on #984 ?

@Shashankft9
Copy link
Member Author

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?

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

if f.Runtime == "go" || f.Runtime == "rust" {
if f.Runtime == "rust" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builder should be "pack".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but empty string probably would work too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

@Shashankft9 unittests are failing.

@matejvasek
Copy link
Contributor

@zroubalik PTAL

@matejvasek
Copy link
Contributor

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

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

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Or am I missing anything?

@matejvasek
Copy link
Contributor

matejvasek commented Dec 14, 2022

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
Copy link
Contributor

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.

@github-actions 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
@lkingland
Copy link
Member

lkingland commented Mar 20, 2023

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?

@github-actions 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
@Shashankft9
Copy link
Member Author

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.

Copy link
Member

@lkingland lkingland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

if f.Runtime == "go" || f.Runtime == "rust" {
if f.Runtime == "rust" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

@knative-prow-robot 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 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
Copy link
Member Author

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
Copy link

CLA Missing ID CLA Not Signed

Signed-off-by: Matej Vasek <mvasek@redhat.com>
@matejvasek
Copy link
Contributor

PTAL @lance

@matejvasek
Copy link
Contributor

@Shashankft9 I pushed on fixup.

@lance
Copy link
Member

lance commented Jul 26, 2023

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
Copy link

knative-prow bot commented Jul 26, 2023

@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
Copy link
Contributor

/approve
/lgtm

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

knative-prow bot commented Jul 28, 2023

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

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

@knative-prow 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 knative-prow bot merged commit 2463202 into knative:main Jul 28, 2023
37 of 38 checks passed
@Shashankft9
Copy link
Member Author

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

@matejvasek
Copy link
Contributor

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

@matejvasek
Copy link
Contributor

matejvasek commented Jul 28, 2023

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.

matejvasek added a commit to matejvasek/faas that referenced this pull request Sep 27, 2023
* 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>
knative-prow bot pushed a commit that referenced this pull request Oct 2, 2023
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On cluster build: Not working for a golang function
6 participants