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.

Sign up for GitHub

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

Jump to bottom

Improve functionRunner interface #599

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Improve functionRunner interface #599

wants to merge 1 commit into from

Conversation

yuwenma
Copy link
Contributor

@yuwenma yuwenma commented Jun 29, 2022

For example, a SetLabel struct that implements Runner interface, have its Owner and Org value automatically parsed from either 1. SetLabels type with owner and org fields or 2. ConfigMap type with Data fields having keys owner and org

type SetLabels struct {
	Owner string
	Org   string
}

FunctionConfig 1

apiVersion:  fn.kpt.dev/v1alpha1
kind: SetLabel
metadata:
   name: custom
owner: kpt
org: google

FunctionConfig 2

apiVersion: v1
kind: ConfigMap
metadata:
   name: custom
data:
  owner: kpt 
  org: google

@yuwenma
Copy link
Contributor Author

yuwenma commented Jun 29, 2022

cc @zyy98

@yuwenma yuwenma force-pushed the runner branch 3 times, most recently from 3e2e7d3 to 08a35ef Compare June 29, 2022 01:01
@yuwenma yuwenma marked this pull request as draft June 29, 2022 01:33
justinsb
justinsb reviewed Jul 12, 2022
View reviewed changes
Copy link
Contributor

@justinsb justinsb left a comment

Choose a reason for hiding this comment

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

A few thoughts, but I like the direction!

go/fn/examples/example_asmain_runner_test.go
// `items` is parsed from the STDIN "ResourceList.Items".
// `functionConfig` is from the STDIN "ResourceList.FunctionConfig". The value is parsed from a `CustomFnConfig` type
// "owner" and "org" field.
func (r *CustomFnConfig) Run(ctx *fn.Context, functionConfig *fn.KubeObject, items fn.KubeObjects) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need functionConfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking about some special handling. Like set-namespace supports variant constructor by accepting a kptfile.kpt.dev named "ConfigMap"

go/fn/examples/example_asmain_runner_test.go

// This example uses a CustomFnConfig object, which implements `Runner.Run` methods.
func Example_asMainCustomFnConfig() {
file, _ := os.Open("./data/runner-customFnConfig.yaml")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd move these to golden tests, because that keeps the function itself cleaner and shows how to test!

go/fn/examples/example_asmain_runner_test.go
// `functionConfig` is from the STDIN "ResourceList.FunctionConfig". The value has been assigned to the r.Labels
// the functionConfig is validated to have kind "SetLabels" and apiVersion "fn.kpt.dev/v1alpha1"
func (r *SetLabels) Run(ctx *fn.Context, functionConfig *fn.KubeObject, items []*fn.KubeObject) {
// `functionConfig` is from the STDIN "ResourceList.FunctionConfig". The value is parsed from a `ConfigMap` type
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we switching away from the "CRD" style for backwards compatibility? I find the ConfigMap approach much harder to think about, and we don't get any type-assistance.

I'd rather think about how to map from a ConfigMap to a CRD, which I think will also help with kptdev/kpt#3351

go/fn/functionrunner.go
@@ -15,5 +15,5 @@
package fn

type Runner interface {
Run(context *Context, functionConfig *KubeObject, items []*KubeObject)
Run(context *Context, functionConfig *KubeObject, items KubeObjects)
Copy link
Contributor

Choose a reason for hiding this comment

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

I like it!

go/fn/runnerProcessor.go
switch fnRunnerElem.Field(i).Kind() {
case reflect.String:
for k, v := range data {
lowerKey := strings.ToLower(k)
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this approach. We might consider also using the json tag to get the ConfigMap field name.

@yuwenma yuwenma mentioned this pull request Jul 29, 2022
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinsb justinsb justinsb left review comments

@droot droot Awaiting requested review from droot

@mengqiy mengqiy Awaiting requested review from mengqiy

@natasha41575 natasha41575 Awaiting requested review from natasha41575

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yuwenma @justinsb