feat: read func classname from the func.yaml file #86

dolfolife · 2022-07-19T20:27:59Z

fixes #85
Signed-off-by: Rodolfo Sanchez [email protected]

vmwclabot · 2022-07-19T20:28:03Z

@dolfolife, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

Commit ec09da2 must be signed by [email protected]

dolfolife · 2022-07-19T20:41:47Z

I still do not know what could be the best path from config -> to build parameters, but I am happy to discuss

buildpacks/java/VERSION

andrew-su · 2022-07-19T21:11:51Z

buildpacks/java/java/build.go

@@ -53,7 +53,7 @@ func (b Build) Build(context libcnb.BuildContext) (libcnb.BuildResult, error) {

 	functionLayer := NewFunction(context.Application.Path,
 		WithLogger(b.Logger),
-		WithFunctionClass(functionClass, isFuncDefDefault),
+		WithFunctionClass(functionClass, isFuncDefDefault, e.Metadata["func_yaml_function_name"].(string)),


I think what you want here is to make func_yaml_function_name to be the default function if it's not empty and isDefaultFuncDefault is false.

I also noticed I mis-named those variables isFuncDefDefault and isDefaultFuncDefault.

They should have been isFuncDefDefined and isDefaultFuncDefined.
cr.resolve returns whether it was explicitly set or not. (false == user did not set the value)

well you want that variable if the ENV is not set, right? the order of precedence is environment -> config -> default

and roger to the isFuncDefault I got confused by the variable name and I did not understand what it was.

andrew-su · 2022-07-19T21:20:58Z

buildpacks/java/tests/detect_test.go

+func withModuleName(value string) func(*libcnb.DetectContext) {
+	return func(dc *libcnb.DetectContext) {
+		dc.Platform.Environment["MODULE_NAME"] = value
+	}
+}
+
+func withFunctionName(value string) func(*libcnb.DetectContext) {
+	return func(dc *libcnb.DetectContext) {
+		dc.Platform.Environment["FUNCTION_NAME"] = value
+	}
+}


These are only relevant to Python

Oh, the only way in Java to set the function name is with BP_FUNCTION or the Spring property?

bbtong · 2022-07-19T21:10:26Z

buildpacks/java/java/detect.go

-					"func_yaml_options": funcYaml.Options,
+					"launch":                  true,
+					"func_yaml_envs":          funcYaml.Envs,
+					"func_yaml_function_name": funcYaml.Name,


I think I would prefer it to just be func_yaml_name to match the other keys better, as you store it in the struct as Name and the getter is getName.

bbtong · 2022-07-19T21:15:20Z

buildpacks/java/java/func_yaml.go

@@ -34,14 +35,33 @@ func ParseFuncYaml(filedir string, logger bard.Logger) FuncYaml {

 	options := optionsToMap(cfg.Options, logger)
 	envs := envsToMap(cfg.Envs, logger)
-
+	name := getName(cfg.Envs, cfg.Name)


Would cfg.Name be cleaner to get the name instead of iterating through cfg.Envs? https://pkg.go.dev/knative.dev/kn-plugin-func#Function

+1 to just using cfg.Name

this was a confusion that I think I have :) where there is an env variable that can be set as the name of the function and that would take precedence over the cfg.Name

cfg pulls from func.yaml though, while that env variable is a Python thing

andrew-su · 2022-07-19T21:26:50Z

buildpacks/java/java/func_yaml.go

+
+	for _, e := range envs {
+		key := *e.Name
+		if key == "FUNCTION_NAME" {


Java doesn't make use of this environment variable, only the python invoker does

is this framework-related logic? should we abstract it?

vmwclabot · 2022-07-20T13:35:37Z

@dolfolife, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

Commit ec09da2 must be signed by [email protected]

vmwclabot · 2022-07-20T21:41:14Z

@dolfolife, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

Commit ec09da2 must be signed by [email protected]

vmwclabot · 2022-07-20T21:46:22Z

@dolfolife, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

Commit ec09da2 must be signed by [email protected]

Signed-off-by: Rodolfo Sanchez <[email protected]>

vmwclabot · 2022-08-03T13:31:30Z

@dolfolife, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

Commit 2cb3f85 must be signed by [email protected]

bbtong · 2022-08-26T04:57:47Z

@dolfolife Has this gone stale or should it be merged in still?

vmwclabot added the dco-required label Jul 19, 2022

dolfolife marked this pull request as draft July 19, 2022 20:30

andrew-su reviewed Jul 19, 2022

View reviewed changes

bbtong reviewed Jul 19, 2022

View reviewed changes

andrew-su reviewed Jul 19, 2022

View reviewed changes

andrew-su force-pushed the main branch from 653404c to 0b07e8c Compare July 20, 2022 16:20

Rodolfo Sanchez added 7 commits July 25, 2022 15:29

feat: read func classname from the func.yaml file

4c24bc9

Signed-off-by: Rodolfo Sanchez <[email protected]>

revert version change

5ad84e7

Signed-off-by: Rodolfo Sanchez <[email protected]>

feat: use overwrite attribute

870d393

Signed-off-by: Rodolfo Sanchez <[email protected]>

change name to func_yaml_name

5de1a8e

Signed-off-by: Rodolfo Sanchez <[email protected]>

delete check env for function name

d810ca0

Signed-off-by: Rodolfo Sanchez <[email protected]>

doc: delete function class

a4c9426

Signed-off-by: Rodolfo Sanchez <[email protected]>

fix name of the env variable

658bdb4

Signed-off-by: Rodolfo Sanchez <[email protected]>

dolfolife force-pushed the feat-use-func-yaml-name branch from bec819f to 658bdb4 Compare July 25, 2022 20:30

vmwclabot removed the dco-required label Jul 25, 2022

Merge branch 'main' into feat-use-func-yaml-name

2cb3f85

vmwclabot added the dco-required label Aug 3, 2022

dolfolife closed this Aug 29, 2022

dolfolife deleted the feat-use-func-yaml-name branch August 29, 2022 18:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: read func classname from the func.yaml file #86

feat: read func classname from the func.yaml file #86

dolfolife commented Jul 19, 2022

vmwclabot commented Jul 19, 2022

dolfolife commented Jul 19, 2022

andrew-su Jul 19, 2022 •

edited

Loading

andrew-su Jul 19, 2022

dolfolife Jul 20, 2022

andrew-su Jul 19, 2022

dolfolife Jul 20, 2022

bbtong Jul 19, 2022

bbtong Jul 19, 2022

andrew-su Jul 19, 2022

dolfolife Jul 20, 2022

bbtong Jul 20, 2022

andrew-su Jul 19, 2022

dolfolife Jul 20, 2022

vmwclabot commented Jul 20, 2022

vmwclabot commented Jul 20, 2022

vmwclabot commented Jul 20, 2022

vmwclabot commented Aug 3, 2022

bbtong commented Aug 26, 2022

feat: read func classname from the func.yaml file #86

feat: read func classname from the func.yaml file #86

Conversation

dolfolife commented Jul 19, 2022

vmwclabot commented Jul 19, 2022

dolfolife commented Jul 19, 2022

andrew-su Jul 19, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmwclabot commented Jul 20, 2022

vmwclabot commented Jul 20, 2022

vmwclabot commented Jul 20, 2022

vmwclabot commented Aug 3, 2022

bbtong commented Aug 26, 2022

andrew-su Jul 19, 2022 •

edited

Loading