Fix typos and clean up#938
Conversation
|
I'm not quite sure why we test for |
andresmgot
left a comment
andresmgot
left a comment
There was a problem hiding this comment.
Hi @dannyqiu, thanks for this PR! I've sent a few comments.
| 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 |
There was a problem hiding this comment.
kubeless-non-rbac.jsonnet has its own task, why are you adding it here?
There was a problem hiding this comment.
Since kubeless.yaml is generated from kubeless-non-rbac.jsonnet, it should be a dependency when compiling.
| } | ||
| req.SetHeader("event-id", eventID) | ||
| req.SetHeader("event-time", timestamp.String()) | ||
| req.SetHeader("event-time", timestamp.Format(time.RFC3339)) |
There was a problem hiding this comment.
👍 Agree with the format
pkg/utils/kubelessutil.go
Outdated
| }, | ||
| ) | ||
| } else { | ||
| return fmt.Errorf("Expected non-empty handler and non-empty function content") |
There was a problem hiding this comment.
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.
|
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 https://github.com/kubeless/kubeless/blob/366318ecd3a85e3acd21d46351ef084ff0cf6595/examples/Makefile#L308 |
examples/Makefile
Outdated
| # Verify event context | ||
| logs=`kubectl logs -l function=python-nats`; \ | ||
| echo $$logs | grep -q "event-time.*UTC" && \ | ||
| echo $$logs | grep -q "event-time.*Z" && \ |
There was a problem hiding this comment.
your changes doesn't affect to nats
examples/Makefile
Outdated
| # Verify event context | ||
| logs=`kubectl logs -l function=python-kinesis`; \ | ||
| echo $$logs | grep -q "event-time.*UTC" && \ | ||
| echo $$logs | grep -q "event-time.*Z" && \ |
There was a problem hiding this comment.
your changes doesn't affect to kinesis either
examples/Makefile
Outdated
| # 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" && \ |
examples/Makefile
Outdated
| # 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" && \ |
examples/Makefile
Outdated
| # 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" && \ |
examples/Makefile
Outdated
| # 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" && \ |
examples/Makefile
Outdated
| # 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" && \ |
|
Sorry, I didn't see the updated changes. Thanks for the patch! |
2 participants
Issue Ref: [Issue number related to this PR or None]
Description:
TODOs: