-
Notifications
You must be signed in to change notification settings - Fork 136
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
Add go function invocation runtime as a package #1451
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lance 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 |
Codecov ReportBase: 63.00% // Head: 63.27% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1451 +/- ##
==========================================
+ Coverage 63.00% 63.27% +0.26%
==========================================
Files 74 75 +1
Lines 10783 10785 +2
==========================================
+ Hits 6794 6824 +30
+ Misses 3430 3404 -26
+ Partials 559 557 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
/cc @knative/func-reviewers @knative/func-writers @knative/functions-wg-leads |
This change takes the Golang function invocation framework from boson-project/packs and adds that code as a package to func itself. We should consider instead if it makes more sense to move this to https://github.com/knative/pkg. 🎁 Adds the Go function runtime from https://github.com/boson-project/packs/tree/main/buildpacks/go 🎁 Updates templates to use the func package /kind enhancement Signed-off-by: Lance Ball <lball@redhat.com>
Consider backporting to |
Note that currently, the s2i golang image does not support non-standard build targets. See: sclorg/golang-container#48 |
This allows, for example, a user to use the Google builder with their Go functions. A working example with no changes to the code outside of this PR...
Here is the same Go function being built and deployed on-cluster using the Google builder.
|
What would we need to do to make it work. |
Yes that would fix it, but I really do like having |
There has also been some talk of mapping |
Please do not merge this just yet. I think with some changes it will be quite a nice addition, but in this current form it's got a little bit more to add to be complete. I am so sorry, but I am out sick with covid, and can not provide much details. In short, this is a nice step in the right direction, but wrapping a function in a process boundary is part of the deployment proces. A function's source code should have no hard dependencies on ops infrastructure. Create a main wrapper from within func is good. But do this as a transient artifact of the build/deploy process. For illustration: what should the dependency tree be of a Function which says "hello world" to an HTTP request? |
I am proposing that we create a new repository https://github.com/knative-sandbox/func-go mapping to |
/hold |
@lance: PR needs rebase. 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. |
I like that this pulls the Go function invocation code out of the buildpack, and how it is now nicely placed right here as a library. Also nice to see the simplification of the template manifest. What I think we need to discuss is at which stage in a function's lifecycle the function's source code is "wrapped" in the process boundary and placed inside a container. In particular, this is part of the build step. So instead of using the template system to do this during function creation, let's do something similar but on build of the container, and as a transient artifact in |
I like this suggestion, but I have two concerns.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A great improvement!
@@ -53,7 +53,10 @@ require ( | |||
knative.dev/serving v0.35.1-0.20221114131921-874ccebb8063 | |||
) | |||
|
|||
require github.com/sabhiram/go-gitignore v0.0.0-20210923224102-525f6e181f06 | |||
require ( | |||
github.com/gorilla/mux v1.8.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/gorilla#gorilla-toolkit
not sure if we should depend on this package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I agree. For the time being, however, I think we can keep it as is until we come up with a suitable alternative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zroubalik also, note that I've moved this code over to https://github.com/knative-sandbox/func-go which will change this PR to just be modifications to the templates.
"knative.dev/func/runtime" | ||
) | ||
|
||
func main() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add some comment here pointing users to handle.go. So they don't try to write the function code here.
"knative.dev/func/runtime" | ||
) | ||
|
||
func main() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add some comment here pointing users to handle.go. So they don't try to write the function code here.
Since this is a bit bigger of a topic, here is a discussion we could use to explore |
Very much appreciate your effort to outline this in a discussion. |
Closing as obsolete |
Changes
This change takes the Golang function invocation framework from boson-project/packs and adds that code as a package to
func
itself. We should consider instead if it makes more sense to move this to https://github.com/knative/pkg.Motivation
Doing this will remove some of the "magic" from how a Golang function works. It also solves a number of problems.
Areas of concern
main()
to be included in the Go function templates (this PR addsfaas/main.go
)go.sum
in the Go function templates, or the user must generate it before an initialfunc build
(this PR addsgo.sum
)func
(unless the package is moved toknative.dev/pkg/runtime
)TODO
replace
directives if/when this PR lands (is there a better way to do this?)/kind enhancement
Fixes: #809
Related: #984
Related: #1445
Release notes