Fix typos and clean up #938

dannyqiu · 2018-10-30T03:16:33Z

Issue Ref: [Issue number related to this PR or None]

Description:

Fix documentation typos
Clean up old binaries
Standardize timestamp format (RFC3339 so that runtimes can more easily parse)

TODOs:

Ready to review
Automated Tests
Docs

dannyqiu · 2018-10-30T03:21:40Z

I'm not quite sure why we test for TestEnsureDeploymentWithoutFuncNorHandler and TestEnsureDeploymentWithoutFunc, where functions would be deployed that do nothing. I think this leads to silent failures that users should actually be aware of?

andresmgot

Hi @dannyqiu, thanks for this PR! I've sent a few comments.

andresmgot · 2018-10-30T08:27:44Z

Makefile

@@ -38,7 +38,7 @@ binary-cross:

 all-yaml: kubeless.yaml kubeless-non-rbac.yaml kubeless-openshift.yaml kafka-zookeeper.yaml

-kubeless.yaml: kubeless.jsonnet
+kubeless.yaml: kubeless.jsonnet kubeless-non-rbac.jsonnet


kubeless-non-rbac.jsonnet has its own task, why are you adding it here?

Since kubeless.yaml is generated from kubeless-non-rbac.jsonnet, it should be a dependency when compiling.

andresmgot · 2018-10-30T08:29:23Z

cmd/kubeless/function/call.go

@@ -97,7 +97,7 @@ var callCmd = &cobra.Command{
 			logrus.Fatalf("Unable to generate ID %v", err)
 		}
 		req.SetHeader("event-id", eventID)
-		req.SetHeader("event-time", timestamp.String())
+		req.SetHeader("event-time", timestamp.Format(time.RFC3339))


👍 Agree with the format

andresmgot · 2018-10-30T08:37:05Z

pkg/utils/kubelessutil.go

@@ -641,6 +641,8 @@ func EnsureFuncDeployment(client kubernetes.Interface, funcObj *kubelessApi.Func
 				Value: dpm.Spec.Template.Spec.Containers[0].Resources.Limits.Memory().String(),
 			},
 		)
+	} else {
+		return fmt.Errorf("Expected non-empty handler and non-empty function content")


We should not return an error here because there is the case in which the user specifies an already built image (custom) that doesn't require a handler and function. For example: kubeless function deploy foo --runtime-image=nginx is a valid command. I am okay if we print something here in the logs though because probably something weird is happening.

andresmgot

👍

andresmgot · 2018-10-31T10:45:25Z

BTW @dannyqiu to fix the CI you need to modify the tests that are looking for the old timestamp format. You need to modify the tests that looks for event-time.*UTC (post-python, post-nodejs, post-go, post-ruby (UTC is not printed anymore, you can substitute that with Z). Those are in the examples/Makefile file:

https://github.com/kubeless/kubeless/blob/366318ecd3a85e3acd21d46351ef084ff0cf6595/examples/Makefile#L308
https://github.com/kubeless/kubeless/blob/366318ecd3a85e3acd21d46351ef084ff0cf6595/examples/Makefile#L328

andresmgot

You modified more examples than needed, please revert those changes for the cases that are not affected. Thanks for applying the changes!

andresmgot · 2018-10-31T16:31:54Z

examples/Makefile

@@ -407,7 +407,7 @@ python-nats-verify:
 	$$found
 	# Verify event context
 	logs=`kubectl logs -l function=python-nats`; \
-	echo $$logs | grep -q "event-time.*UTC" && \
+	echo $$logs | grep -q "event-time.*Z" && \


your changes doesn't affect to nats

andresmgot · 2018-10-31T16:32:06Z

examples/Makefile

@@ -441,7 +441,7 @@ python-kinesis-verify:
 	$$found
 	# Verify event context
 	logs=`kubectl logs -l function=python-kinesis`; \
-	echo $$logs | grep -q "event-time.*UTC" && \
+	echo $$logs | grep -q "event-time.*Z" && \


your changes doesn't affect to kinesis either

andresmgot · 2018-10-31T16:32:16Z

examples/Makefile

@@ -475,7 +475,7 @@ python-kinesis-multi-record-verify:
 	$$found
 	# Verify event context
 	logs=`kubectl logs -l function=python-kinesis-multi-record`; \
-	echo $$logs | grep -q "event-time.*UTC" && \
+	echo $$logs | grep -q "event-time.*Z" && \


the same for kinesis

andresmgot · 2018-10-31T16:32:25Z

examples/Makefile

@@ -519,7 +519,7 @@ nats-python-func1-topic-test-verify:
 	$$found
 	# Verify event context
 	logs=`kubectl logs -l function=nats-python-func1-topic-test`; \
-	echo $$logs | grep -q "event-time.*UTC" && \
+	echo $$logs | grep -q "event-time.*Z" && \


the same for nats

andresmgot · 2018-10-31T16:32:36Z

examples/Makefile

@@ -545,7 +545,7 @@ nats-python-func2-topic-test-verify:
 	$$found
 	# Verify event context
 	logs=`kubectl logs -l function=nats-python-func2-topic-test`; \
-	echo $$logs | grep -q "event-time.*UTC" && \
+	echo $$logs | grep -q "event-time.*Z" && \


the same for nats

andresmgot · 2018-10-31T16:32:53Z

examples/Makefile

@@ -571,7 +571,7 @@ nats-python-func-multi-topic-verify:
 	$$found
 	# Verify event context
 	logs=`kubectl logs -l function=nats-python-func-multi-topic`; \
-	echo $$logs | grep -q "event-time.*UTC" && \
+	echo $$logs | grep -q "event-time.*Z" && \


andresmgot · 2018-10-31T16:32:58Z

examples/Makefile

@@ -593,7 +593,7 @@ nats-python-func-multi-topic-verify:
 	$$found
 	# Verify event context
 	logs=`kubectl logs -l function=nats-python-func-multi-topic`; \
-	echo $$logs | grep -q "event-time.*UTC" && \
+	echo $$logs | grep -q "event-time.*Z" && \


and the same

andresmgot · 2018-11-06T11:26:20Z

Sorry, I didn't see the updated changes. Thanks for the patch!

dannyqiu added 4 commits October 29, 2018 23:06

Remove old binaries from before trigger controller refactor

5c74086

Recompile kubeless.yaml when kubeless-non-rbac.jsonnet changes

30070ea

Put time in RFC3339 format

d87903c

Fix typos in documentation

57721f5

andresmgot reviewed Oct 30, 2018

View reviewed changes

Warn instead of returning an error for empty handler and body

366318e

andresmgot approved these changes Oct 31, 2018

View reviewed changes

andresmgot reviewed Oct 31, 2018

View reviewed changes

Test for RFC3339 timestamp

a73e0ed

andresmgot merged commit bf2871f into vmware-archive:master Nov 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix typos and clean up #938

Fix typos and clean up #938

dannyqiu commented Oct 30, 2018 •

edited by andresmgot

Loading

dannyqiu commented Oct 30, 2018

andresmgot left a comment

andresmgot Oct 30, 2018

dannyqiu Oct 30, 2018

andresmgot Oct 30, 2018

andresmgot Oct 30, 2018

andresmgot left a comment

andresmgot commented Oct 31, 2018 •

edited

Loading

andresmgot left a comment •

edited

Loading

andresmgot Oct 31, 2018

andresmgot Oct 31, 2018

andresmgot Oct 31, 2018

andresmgot Oct 31, 2018

andresmgot Oct 31, 2018

andresmgot Oct 31, 2018

andresmgot Oct 31, 2018

andresmgot commented Nov 6, 2018

Fix typos and clean up #938

Fix typos and clean up #938

Conversation

dannyqiu commented Oct 30, 2018 • edited by andresmgot Loading

dannyqiu commented Oct 30, 2018

andresmgot left a 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

andresmgot left a comment

Choose a reason for hiding this comment

andresmgot commented Oct 31, 2018 • edited Loading

andresmgot left a comment • 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

andresmgot commented Nov 6, 2018

dannyqiu commented Oct 30, 2018 •

edited by andresmgot

Loading

andresmgot commented Oct 31, 2018 •

edited

Loading

andresmgot left a comment •

edited

Loading