Added support for Docker, GitHub Actions and graceful shutdown #677

ad-m · 2021-02-11T22:24:52Z

Close #637
Depends on #676 due formatting issues
Follow-up #637 due deleted fork

kherock

Hey, sorry I missed this before leaving a comment on your old PR. Also don't feel offended by me opening #681, those were honestly changes I've had in my own personal fork since June.

My comment here still stands. I'm not particularly attached to 4568 as the default port, it's just what we've always had. Regardless, I really think the Docker image should be consistent with the documented default port.

jamhall · 2021-02-12T07:03:34Z

README.md

 [![Dependency Status](https://david-dm.org/jamhall/s3rver/status.svg)](https://david-dm.org/jamhall/s3rver)
 [![Devdependency Status](https://david-dm.org/jamhall/s3rver/dev-status.svg)](https://david-dm.org/jamhall/s3rver?type=dev)

 S3rver is a lightweight server that responds to **some** of the same calls [Amazon S3](http://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/S3.html) responds to. It is extremely useful for testing S3 in a sandbox environment without actually making calls to Amazon.

-The goal of S3rver is to minimise runtime dependencies and be more of a development tool to test S3 calls in your code rather than a production server looking to duplicate S3 functionality.
+The goal of S3rver is to minimize runtime dependencies and be more of a development tool to test S3 calls in your code rather than a production server looking to duplicate S3 functionality.


This isn't a typo or a spelling mistake. I'm British, so I use the British English spelling of words! 😃

It makes sense to use the same English spelling conventions throughout the docs, but surely there would be more changes to add too. Consider dropping this change and creating a different PR to address English language spelling conventions.

leontastic

I'm 100% fine with the Docker stuff, I would approve a PR containing only those changes. However I think GitHub actions were handled in #681, and I am unconvinced about the need for graceful shutdown.

@ad-m Thanks for the initiative you took on this PR. If you can address my concerns in the review I'd be happy to come back to review this PR again.

leontastic · 2021-03-04T22:33:15Z

.dockerignore

@@ -0,0 +1,2 @@
+node_modules
+.git


Missing newline

leontastic · 2021-03-04T22:43:49Z

.github/workflows/javascript.yaml

@@ -0,0 +1,26 @@
+name: JavaScript files


This looks good but workflows have been addressed in #681.

Current source: https://github.com/jamhall/s3rver/blob/main/.github/workflows/ci.yml

I think the existing ci workflow definitions are pretty good. I would drop this and make changes to the existing definitions instead, if you have any improvements to suggest.

leontastic · 2021-03-04T22:47:32Z

README.md

@@ -3,12 +3,13 @@
 [![NPM](https://nodei.co/npm/s3rver.png)](https://nodei.co/npm/s3rver/)

 [![Build Status](https://api.travis-ci.org/jamhall/s3rver.png)](https://travis-ci.org/jamhall/s3rver)
+![JavaScript files](https://github.com/jamhall/s3rver/workflows/JavaScript%20files/badge.svg)


It looks like we don't have this badge for the existing CI workflow. If you're dropping the JavaScript files workflow from this PR maybe change this to use the CI badge instead of removing this line altogether.

![JavaScript files](https://github.com/jamhall/s3rver/workflows/CI/badge.svg)

Although adding the CI workflow badge could be its own PR. It wouldn't be related to the rest of the work here.

leontastic · 2021-03-04T23:49:45Z

README.md

 [![Dependency Status](https://david-dm.org/jamhall/s3rver/status.svg)](https://david-dm.org/jamhall/s3rver)
 [![Devdependency Status](https://david-dm.org/jamhall/s3rver/dev-status.svg)](https://david-dm.org/jamhall/s3rver?type=dev)

 S3rver is a lightweight server that responds to **some** of the same calls [Amazon S3](http://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/S3.html) responds to. It is extremely useful for testing S3 in a sandbox environment without actually making calls to Amazon.

-The goal of S3rver is to minimise runtime dependencies and be more of a development tool to test S3 calls in your code rather than a production server looking to duplicate S3 functionality.
+The goal of S3rver is to minimize runtime dependencies and be more of a development tool to test S3 calls in your code rather than a production server looking to duplicate S3 functionality.


It makes sense to use the same English spelling conventions throughout the docs, but surely there would be more changes to add too. Consider dropping this change and creating a different PR to address English language spelling conventions.

leontastic · 2021-03-04T23:58:38Z

bin/s3rver.js

+  const { address, port } = await server.run();
+
+  process.on('SIGINT', async function onSigint() {
+    console.info('Got SIGINT. Graceful shutdown ', new Date().toISOString());


Not sure this is a useful output. Would rather exit quietly.

leontastic · 2021-03-05T00:03:43Z

bin/s3rver.js

+  const server = new S3rver(opts);
+  const { address, port } = await server.run();
+
+  process.on('SIGINT', async function onSigint() {


From Node docs:

'SIGTERM' and 'SIGINT' have default handlers on non-Windows platforms that reset the terminal mode before exiting with code 128 + signal number. If one of these signals has a listener installed, its default behavior will be removed (Node.js will no longer exit).

I think this means you must call process.exit inside these handlers or else the node process won't be terminated via SIGINT or SIGTERM, you would have to send a SIGKILL to terminate the process.

leontastic · 2021-03-05T00:38:00Z

lib/s3rver.js

@@ -145,6 +161,8 @@ class S3rver extends Koa {

      const { address, port, ...listenOptions } = this.serverOptions;
      this.httpServer = await this.listen(port, address, listenOptions);
+      stoppable(this.httpServer);


I think I understand what .stop patched by stoppable is supposed to do, but I'm having trouble understanding why S3rver would benefit from it compared to using the .close function.

S3rver is intended for integration testing, so when you call .close you would probably want to perform cleanup as quickly as possible and do not care about open connections. If there are in-flight requests or Keep-Alive connections still open when you call .close, you probably forgot to wait for your requests to end before cleaning up your tests, and you would want to receive errors about prematurely closed connections rather than have these connections silently cleaned up on close.

If you are running the process independently in the background or in a container, prematurely closed connections would indicate that you closed your server earlier than your tests completed, which IMO is fine and should be expected. It would be a little unexpected for that server to remain alive while my tests finish up their current requests, only to reject future requests and fail the remainder of the tests.

So I think there are no use cases where .stop's graceful functionality would be useful, and would argue against introducing stoppable for this reason. However if you can suggest a use case where graceful shutdown is needed I may reverse my opinion on this.

ad-m and others added 6 commits February 11, 2021 23:26

Add initial support for Docker and GitHub Actions

098cc46

Added support graceful shutdown

a22d023

Bump NodeJS version in Docker

7bc0b6f

Bump NodeJS version in GitHub Actions

0c0d99a

Fix formatting

130dd70

Fix passing socket on close server

cf6bb7e

ad-m force-pushed the pr-637 branch 6 times, most recently from b138e28 to 0ad4a76 Compare February 11, 2021 23:03

Apply review comments

f725751

ad-m force-pushed the pr-637 branch from 0ad4a76 to f725751 Compare February 11, 2021 23:04

kherock reviewed Feb 12, 2021

View reviewed changes

fishcharlie mentioned this pull request Feb 12, 2021

Implement GitHub Actions w/ codecov reports #681

Merged

jamhall reviewed Feb 12, 2021

View reviewed changes

Base automatically changed from master to main February 12, 2021 14:09

leontastic requested changes Mar 5, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for Docker, GitHub Actions and graceful shutdown #677

Added support for Docker, GitHub Actions and graceful shutdown #677

ad-m commented Feb 11, 2021 •

edited

Loading

kherock left a comment

jamhall Feb 12, 2021

leontastic Mar 4, 2021

leontastic left a comment

leontastic Mar 4, 2021

leontastic Mar 4, 2021

leontastic Mar 4, 2021

leontastic Mar 4, 2021

leontastic Mar 4, 2021

leontastic Mar 5, 2021

leontastic Mar 5, 2021

		@@ -0,0 +1,2 @@
		node_modules
		.git

Added support for Docker, GitHub Actions and graceful shutdown #677

Are you sure you want to change the base?

Added support for Docker, GitHub Actions and graceful shutdown #677

Conversation

ad-m commented Feb 11, 2021 • edited Loading

kherock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leontastic 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ad-m commented Feb 11, 2021 •

edited

Loading