feat: add lifecycle and endpoint customization #203

lance · 2023-03-24T02:05:41Z

This is still a work in progress. Enables function developers to export init(), shutdown(), liveness() and readiness() functions from the function file, enabling startup behaviors such as a connection to a database or other persistent store, clean shutdowns, and customizable liveness and readiness checks.

Still needed:

Update README with the API
Update hack/run.js to provide a better example (and maybe move from hack/run.js to sample/index.js
Review types.d.ts to make sure we're doing enough typing
Consider any other lifecycle hooks that may be needed
Update the CLI to work with Function objects

Fixes: #142

This is still a work in progress. Enables function developers to export `init()`, `shutdown()`, `liveness()` and `readiness()` functions from the function file, enabling startup behaviors such as a connection to a database or other persistent store, clean shutdowns, and customizable liveness and readiness checks. Signed-off-by: Lance Ball <lball@redhat.com>

codecov-commenter · 2023-03-24T02:08:26Z

Codecov Report

Patch coverage: 84.61% and project coverage change: -1.49 ⚠️

Comparison is base (8883cc0) 92.39% compared to head (d01c69c) 90.90%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #203      +/-   ##
==========================================
- Coverage   92.39%   90.90%   -1.49%     
==========================================
  Files           9        9              
  Lines         263      286      +23     
==========================================
+ Hits          243      260      +17     
- Misses         20       26       +6

Impacted Files	Coverage Δ
index.js	`83.54% <81.81%> (-4.18%)`	⬇️
lib/health-check.js	`100.00% <100.00%> (ø)`
lib/request-handler.js	`100.00% <100.00%> (ø)`

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: Lance Ball <lball@redhat.com>

lance · 2023-03-24T13:28:15Z

@lholmquist I've marked it as a draft, but any feedback you have now is welcome.

lholmquist · 2023-03-24T13:37:57Z

about a month ago i took a quick look at doing something similar here: lholmquist@628fac0

With the quick glance i just took of this PR, it looks like we had the same thoughts, so that's good :)

i'll try to take a better look at comment today

lholmquist · 2023-03-24T13:47:05Z

A thought i just had regarding the shutdown method. The main environment where this framework is running/serving the function is probably going to be in a container, so when the container gets brought down, i wonder how much time a user will have to do something in this method

lance · 2023-03-24T14:29:35Z

i wonder how much time a user will have to do something in this method

Not much that's for sure. I think, however, instead of adding a listener to the server shutdown event, it would be better handled in the way the CLI is doing ON_DEATH. I've played around with some sample functions and this seems like the better approach.

Also cleans up shutdown handling and adds some comments. Signed-off-by: Lance Ball <lball@redhat.com>

Signed-off-by: Lance Ball <lball@redhat.com>

lance · 2023-03-24T16:43:59Z

@lholmquist ok I think this is ready for a first look when you have a chance.

Signed-off-by: Lance Ball <lball@redhat.com>

lholmquist · 2023-03-24T17:38:21Z

README.md

@@ -22,7 +22,39 @@ at `localhost:8080`. The incoming request may be a
 just a simple HTTP GET/POST request. The invoked user function can be
 `async` but that is not required.

-## Function Signatures
+## The Function Interface


It should probably be documented that you can also change the health checks path using either environment variables are the liveness.path/readiness.path properties in the function

Oh right - I forgot about that. At the moment, the Function.liveness.path property will override any environment setting. I wonder if it should be the other way around.

hmmm, good question. i don't have to strong of feelings either way, but probably leaning more towards the way it currently is. function code overrides env variables

We might also want to add port and logLevel as properties on the Function object so that the behavior is the same in terms of precedence. Default is superseded by configuration (in func.yaml, on the command line, or passed options to start()), which is superseded by environment variables.

hmmm, good question. i don't have to strong of feelings either way, but probably leaning more towards the way it currently is. function code overrides env variables

I think the motivation for env variables overriding function config is that the configuration would typically be set up for production environments, and coded into bash scripts or other pipeline tooling. For local development this can be overridden without changing the production settings by adjusting env vars - therefore no changes to the code.

lib/types.d.ts

index.js

Signed-off-by: Lance Ball <lball@redhat.com>

lholmquist

@lance except for my previous comments LGTM!

lance · 2023-03-30T22:29:09Z

@lance except for my previous comments LGTM!

Awesome - but please hold off on merging as I have some additional changes locally that I am still working on. I should get those added tomorrow.

Signed-off-by: Lance Ball <lball@redhat.com>

lance · 2023-03-31T17:09:52Z

@lholmquist OK, I think this is probably good for a final review. I've updated README.md and the TypeScript types.

lholmquist · 2023-04-04T14:30:31Z

lib/health-check.js

+module.exports = function healthCheck(opts) {
+  return (fastify, _, done) => {
+    // Handle health checks
+    fastify.get(opts?.readiness?.path || readinessURL, { logLevel: 'warn' }, opts.readiness || callProtect);


should these default to warn? or should they be whatever is in the config?

@lholmquist done - good catch! Hopefully this PR is good to go now.

Signed-off-by: Lance Ball <lball@redhat.com>

lance · 2023-04-12T14:04:00Z

@lholmquist @matejvasek ptal

lholmquist

Sorry for the delay, i was on PTO last week

LGTM!!

fixup: add test and jsdocs

1205fdb

Signed-off-by: Lance Ball <lball@redhat.com>

lance requested a review from lholmquist March 24, 2023 13:27

lance added 2 commits March 24, 2023 11:49

fixup: add example function in sample/index.js

a14c857

Also cleans up shutdown handling and adds some comments. Signed-off-by: Lance Ball <lball@redhat.com>

fixup: README and comments

3433fd6

Signed-off-by: Lance Ball <lball@redhat.com>

lance marked this pull request as ready for review March 24, 2023 16:43

fixup typo

d3358c6

Signed-off-by: Lance Ball <lball@redhat.com>

lholmquist reviewed Mar 24, 2023

View reviewed changes

lance commented Mar 25, 2023

View reviewed changes

lib/types.d.ts Outdated Show resolved Hide resolved

lance commented Mar 25, 2023

View reviewed changes

lib/types.d.ts Outdated Show resolved Hide resolved

matejvasek reviewed Mar 28, 2023

View reviewed changes

index.js Outdated Show resolved Hide resolved

fixup: use await instead of new Promise()

f74d75a

Signed-off-by: Lance Ball <lball@redhat.com>

lholmquist previously approved these changes Mar 30, 2023

View reviewed changes

lance added 2 commits March 31, 2023 13:00

fixup: add types

56da425

Signed-off-by: Lance Ball <lball@redhat.com>

fixup: update readme

e98b341

Signed-off-by: Lance Ball <lball@redhat.com>

lance dismissed lholmquist’s stale review via e98b341 March 31, 2023 17:04

fixup: update readme

d22cb34

Signed-off-by: Lance Ball <lball@redhat.com>

lholmquist reviewed Apr 4, 2023

View reviewed changes

fixup: use configuration logLevel for health checks

d01c69c

Signed-off-by: Lance Ball <lball@redhat.com>

lholmquist approved these changes Apr 17, 2023

View reviewed changes

lholmquist merged commit ff20b40 into nodeshift:main Apr 17, 2023

github-actions bot mentioned this pull request Apr 17, 2023

chore(main): release 1.1.0 #182

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add lifecycle and endpoint customization #203

feat: add lifecycle and endpoint customization #203

lance commented Mar 24, 2023 •

edited

Loading

codecov-commenter commented Mar 24, 2023 •

edited

Loading

lance commented Mar 24, 2023

lholmquist commented Mar 24, 2023

lholmquist commented Mar 24, 2023

lance commented Mar 24, 2023

lance commented Mar 24, 2023

lholmquist Mar 24, 2023

lance Mar 24, 2023

lholmquist Mar 24, 2023

lance Mar 24, 2023

lance Mar 25, 2023

lholmquist left a comment

lance commented Mar 30, 2023

lance commented Mar 31, 2023

lholmquist Apr 4, 2023

lance Apr 10, 2023

lance commented Apr 12, 2023

lholmquist left a comment

feat: add lifecycle and endpoint customization #203

feat: add lifecycle and endpoint customization #203

Conversation

lance commented Mar 24, 2023 • edited Loading

codecov-commenter commented Mar 24, 2023 • edited Loading

Codecov Report

lance commented Mar 24, 2023

lholmquist commented Mar 24, 2023

lholmquist commented Mar 24, 2023

lance commented Mar 24, 2023

lance commented Mar 24, 2023

lholmquist Mar 24, 2023

Choose a reason for hiding this comment

lance Mar 24, 2023

Choose a reason for hiding this comment

lholmquist Mar 24, 2023

Choose a reason for hiding this comment

lance Mar 24, 2023

Choose a reason for hiding this comment

lance Mar 25, 2023

Choose a reason for hiding this comment

lholmquist left a comment

Choose a reason for hiding this comment

lance commented Mar 30, 2023

lance commented Mar 31, 2023

lholmquist Apr 4, 2023

Choose a reason for hiding this comment

lance Apr 10, 2023

Choose a reason for hiding this comment

lance commented Apr 12, 2023

lholmquist left a comment

Choose a reason for hiding this comment

lance commented Mar 24, 2023 •

edited

Loading

codecov-commenter commented Mar 24, 2023 •

edited

Loading