Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .dockerignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
node_modules
.git
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline

26 changes: 26 additions & 0 deletions .github/workflows/javascript.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
name: JavaScript files
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


on:
push:
pull_request:

jobs:
test:
runs-on: ubuntu-latest
strategy:
matrix:
node: [ '10.x', '12.x', '14.x' ]
steps:
- uses: actions/checkout@v1
- name: Set up Node
uses: actions/setup-node@v1
with:
node-version: '${{ matrix.node }}'
- name: Install
run: npm install
- name: Lint via prettier
run: npm run prettier
- name: Lint via eslint
run: npm run eslint
- name: Test
run: npm run test
9 changes: 9 additions & 0 deletions Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
FROM node:12-alpine
WORKDIR /src
COPY package* ./
ENV NODE_ENV=production
RUN npm install
EXPOSE 8080
VOLUME /data
COPY . .
CMD ["node","bin/s3rver.js", "-p", "8080", "-d", "/data"]
23 changes: 22 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

[![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.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


## Supported methods

Expand All @@ -31,6 +32,8 @@ The goal of S3rver is to minimise runtime dependencies and be more of a developm

## Quick Start

### NodeJS

Install s3rver:

```bash
Expand All @@ -45,6 +48,24 @@ Executing this command for the various options:
$ s3rver --help
```

### Docker

Build image

```bash
$ npm run docker:build
```

You will now have a image `jamhall/s3rver:latest` available.

Executing this command to start server:

```bash
$ npm run docker:start
```

Server will listen on port `8080`.

## Supported clients

Please see [Fake S3's wiki page](https://github.com/jubos/fake-s3/wiki/Supported-Clients) for a list of supported clients.
Expand Down
15 changes: 14 additions & 1 deletion bin/s3rver.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,20 @@ program.on('--help', () => {
program.action(async command => {
const { configureBucket, ...opts } = command.opts();
opts.configureBuckets = configureBucket;
const { address, port } = await new S3rver(opts).run();

const server = new S3rver(opts);
const { address, port } = await server.run();

process.on('SIGINT', async function onSigint() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

console.info('Got SIGINT. Graceful shutdown ', new Date().toISOString());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

await server.stop();
});

process.on('SIGTERM', async function onSigterm() {
console.info('Got SIGTERM. Graceful shutdown ', new Date().toISOString());
await server.stop();
});

console.log();
console.log('S3rver listening on %s:%d', address, port);
});
Expand Down
18 changes: 18 additions & 0 deletions lib/s3rver.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const https = require('https');
const os = require('os');
const path = require('path');
const { callbackify, format, promisify } = require('util');
const stoppable = require('stoppable');

const loggerMiddleware = require('./middleware/logger');
const vhostMiddleware = require('./middleware/vhost');
Expand Down Expand Up @@ -132,6 +133,21 @@ class S3rver extends Koa {
this.store.reset();
}

/**
* Gracefully stops the S3rver server.
*
* @param {Function} [callback] Function called with (err, addressObj) as arguments.
* @returns {this|Promise} The S3rver instance. If no callback function is supplied, a Promise
* is returned.
*/
stop(callback) {
if (typeof callback === 'function') {
return this.httpServer.stop(callback);
} else {
return new Promise(resolve => this.httpServer.stop(resolve));
}
}

/**
* Starts the HTTP server.
*
Expand All @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return this.httpServer.address();
};

Expand Down
6 changes: 5 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,10 @@
"test": "mocha",
"eslint": "eslint --ignore-path .gitignore .",
"prettier": "prettier --ignore-path .gitignore \"**/*.{js,json,md}\"",
"format": "npm run eslint -- --fix && npm run prettier -- --write"
"format": "npm run eslint -- --fix && npm run prettier -- --write",
"docker:build": "docker build -t jamhall/s3rver:latest .",
"docker:push":"docker build -t jamhall/s3rver:latest .",
"docker:start": "docker run -v $(pwd)/data:/data -p 8080:8080 jamhall/s3rver:latest"
},
"main": "lib/s3rver.js",
"files": [
Expand Down Expand Up @@ -58,6 +61,7 @@
"koa-logger": "^3.2.0",
"lodash": "^4.17.5",
"statuses": "^2.0.0",
"stoppable": "^1.1.0",
"winston": "^3.0.0"
},
"devDependencies": {
Expand Down