Skip to content

Enforce a style guide #101

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

Merged
merged 53 commits into from
Jul 13, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
7b36e0e
Add eslint and standard and their plugins
magopian Jul 7, 2017
0e5d9d0
First attempt at having an eslint config for node and the browser
magopian Jul 7, 2017
c2eef44
Add lint-node, lint-node-fix, lint-browser and lint-browser-fix scrip…
magopian Jul 7, 2017
b118bb5
Simplify/reduce the eslintrc files (copy/pasting is bad)
magopian Jul 7, 2017
16ae1dc
Add git hooks to run the linting automatically on each commit/push
magopian Jul 7, 2017
85002f7
Auto fix node files
magopian Jul 7, 2017
00e1210
Auto fix browser files
magopian Jul 7, 2017
c85fdbd
Add an .eslintignore file, ignore js libraries
magopian Jul 10, 2017
ebdfd9d
Lint browser files: fix no-undef
magopian Jul 10, 2017
bcbbb29
Lint browser files: don't error on the usage of 'new' in ES5
magopian Jul 10, 2017
b8d86d8
Lint browser files: use '===' instead of '=='
magopian Jul 10, 2017
bc60c3d
Lint browser files: allow usage of console.error and console.warn
magopian Jul 10, 2017
2839a09
Lint browser files: remove unused formatMemory helper
magopian Jul 10, 2017
03b8cfe
Lint node files: don't disallow console calls on node
magopian Jul 10, 2017
b50b78c
Lint node files: fix no-unused-vars
magopian Jul 10, 2017
2c55981
Lint node files: fix useless escape in a regex
magopian Jul 10, 2017
fe14330
Lint node files: fix no-redeclare
magopian Jul 10, 2017
60f8cc3
Lint node files: fix no-process-exit
magopian Jul 10, 2017
917f501
Add the lint and lint-fix scripts that lint for both browser and node…
magopian Jul 10, 2017
e319915
Lint node files: fix no-case-declarations
magopian Jul 10, 2017
9095f88
Lint node files: fix no-useless-return
magopian Jul 10, 2017
60c7d03
Lint node files: fix eslint parsing error
magopian Jul 10, 2017
0354542
Lint node files: autofix issues now that eslint can parse boot.js
magopian Jul 10, 2017
58e7eb6
Lint node files: fix no-func-assign: added an exception
magopian Jul 10, 2017
3185ba8
Lint node files: fix camel case
magopian Jul 10, 2017
fc483f9
Lint node files: fix no-func-assign: added an exception
magopian Jul 10, 2017
c770f80
Lint node files: fix another camel case issue
magopian Jul 10, 2017
609be0d
Lint node files: fix another camel case issue
magopian Jul 10, 2017
ba920f5
Lint node files: 0600 isn't the same as 600. Use a string instead for…
magopian Jul 10, 2017
9d6d70a
Lint node files: use camelcase for constructors
magopian Jul 10, 2017
ad5c216
Lint node files: fix node/no-unsupported-features by adding an exception
magopian Jul 10, 2017
0eccbb3
Lint node files: fix the handle-callback-err by logging the errors
magopian Jul 10, 2017
6a6883b
Lint browser files: set comma-dangle to 'off' which is clearer that i…
magopian Jul 10, 2017
029f762
Revert "Lint browser files: remove unused formatMemory helper"
magopian Jul 10, 2017
65a71e8
Revert "Lint browser files: fix no-undef"
magopian Jul 10, 2017
75b6ca5
Lint browser files: add an exception for the formatMemory unused helper
magopian Jul 10, 2017
6a6d5d4
Lint browser files: add globals in eslint's configuration
magopian Jul 10, 2017
60d32b6
Revert "Add git hooks to run the linting automatically on each commit…
magopian Jul 10, 2017
c2c8d16
Lint: allow comma-dangle everywhere for node files, only in array and…
magopian Jul 10, 2017
d1d0065
Remove redundant log message when throwing
magopian Jul 11, 2017
c45b95e
Change 'let' to 'const' for requires
magopian Jul 11, 2017
3983077
Better failure log messages
magopian Jul 11, 2017
cb8393c
Revert "Lint node files: fix camel case"
magopian Jul 11, 2017
bab1b2c
Revert "Lint node files: fix another camel case issue"
magopian Jul 11, 2017
419a6d0
Allow non-camelcase names
magopian Jul 11, 2017
097403b
Add a [fail] prefix to a log message
magopian Jul 11, 2017
3cc9ec0
Move the error treatment up so it's the first thing done in a function
magopian Jul 11, 2017
034ac08
Remove useless lint-browser* and lint-node* scripts in package.json
magopian Jul 11, 2017
2ce2601
Add the 'no-var' eslint rule for node files, replace them by let or c…
magopian Jul 11, 2017
0df254c
Re-lint after the rebase
magopian Jul 11, 2017
d6bd3d5
@nt1m review
magopian Jul 13, 2017
d26f8c2
@nt1m review
magopian Jul 13, 2017
6f75463
@jankeromnes review
magopian Jul 13, 2017
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
1 change: 1 addition & 0 deletions .eslintignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
static/js/*.min.js
36 changes: 36 additions & 0 deletions .eslintrc-browser.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
module.exports = {
"env": {
"browser": true,
"es6": false
Copy link
Member

Choose a reason for hiding this comment

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

Quite funny that the eslintrc.js files don't actually respect the eslint rules :D

Property names should either have no quotes or single quotes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe it's because you can either use a js file, or plain json? I don't have an opinion on this matter. Also for some reason those configuration files are auto-ignored by eslint.

},
"extends": [
"eslint:recommended",
"standard"
],
"globals": {
"$": true,
"Dygraph": true,
"Scout": true,
"ajaxForm": true,
"updateFormStatus": true,
},
"rules": {
// Override some of standard js rules.
"semi": ["error", "always"],
"comma-dangle": [
"error", {
"arrays": "only-multiline",
"objects": "only-multiline",
"imports": "never",
"exports": "never",
"functions": "never",
}
],

// Override some eslint base rules because we're using ES5.
"no-new": "off",

// Custom rules.
"no-console": ["error", {"allow": ["warn", "error"]}],
}
};
25 changes: 25 additions & 0 deletions .eslintrc-node.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
module.exports = {
"env": {
"browser": false,
"es6": true,
"node": true
},
"extends": [
"eslint:recommended",
"plugin:node/recommended",
"standard"
],
"plugins": [
"node"
],
"rules": {
// Override some of standard js rules
"semi": ["error", "always"],
"comma-dangle": ["error", "only-multiline"],
"camelcase": "off",
"no-var": "error",

// Override some eslint base rules because we're using node.
"no-console": "off",
}
};
1 change: 0 additions & 1 deletion api/hosts-api.js
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,6 @@ hostAPI.post({
// No authentication.
response.statusCode = 403; // Forbidden
response.json({ error: 'Unauthorized' }, null, 2);
return;
Copy link
Member

Choose a reason for hiding this comment

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

Interesting. This return was indeed unnecessary, but I added it as an indication that there should be no further instructions in this block (below, there are some inline functions, but there shouldn't be any other code or logic that risks calling response.json() or response.end() a second time). This is just a random remark, I'm fine with this change.


function createHost () {
getHostProperties(properties => {
Expand Down
1 change: 0 additions & 1 deletion api/user-api.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ const configurations = require('../lib/configurations');
const db = require('../lib/db');
const log = require('../lib/log');
const machines = require('../lib/machines');
const users = require('../lib/users');

// API resource to manage a Janitor user.
const userAPI = module.exports = selfapi({
Expand Down
6 changes: 3 additions & 3 deletions app.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ boot.executeInParallel([
boot.ensureHttpsCertificates,
boot.ensureDockerTlsCertificates
], () => {

// You can customize these values in './db.json'.
const hostname = db.get('hostname', 'localhost');
const https = db.get('https');
Expand Down Expand Up @@ -495,11 +494,12 @@ boot.executeInParallel([
}

let key = '';
let match;
switch (data.name) {
case 'cloud9':
// Extract a valid SSH public key from the user's input.
// Regex adapted from https://gist.github.com/paranoiq/1932126.
var match = data.key.match(/ssh-rsa [\w+\/]+[=]{0,3}/);
match = data.key.match(/ssh-rsa [\w+/]+[=]{0,3}/);
if (!match) {
return end({ status: 'error', message: 'Invalid SSH key' });
}
Expand All @@ -509,7 +509,7 @@ boot.executeInParallel([

case 'cloud9user':
// Cloud9 usernames consist of lowercase letters, numbers and '_' only.
var match = data.key.trim().match(/^[a-z0-9_]+$/);
match = data.key.trim().match(/^[a-z0-9_]+$/);
if (!match) {
return end({ status: 'error', message: 'Invalid Cloud9 username' });
}
Expand Down
12 changes: 5 additions & 7 deletions join.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,8 @@ const sessions = require('./lib/sessions');
const hostname = db.get('hostname', 'localhost');

if (!hostname || hostname === 'localhost') {
log('[fail] cannot join cluster as [hostname = ' + hostname + ']: ' +
throw new Error('Cannot join cluster as [hostname = ' + hostname + ']: ' +
'please fix the hostname in ./db.json and try again');
return;
}

log('[ok] will try to join cluster as [hostname = ' + hostname + ']');
Expand Down Expand Up @@ -256,18 +255,17 @@ function routeRequest (proxyParameters, request, response) {
switch (proxy) {
case 'https':
routes.webProxy(request, response, { port, path });
return;
break;

case 'none':
const url = 'https://' + hostname + ':' + port + path;
routes.redirect(response, url);
return;
routes.redirect(response, 'https://' + hostname + ':' + port + path);
break;

default:
log('[fail] unsupported proxy type:', proxy);
response.statusCode = 500; // Internal Server Error
response.end();
return;
Copy link
Member

Choose a reason for hiding this comment

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

It's inconsistent that we have return; in the above case statement, but not here. Can you instead use break; here and above to remove this inconsistency?

break;
}
}

Expand Down
6 changes: 3 additions & 3 deletions lib/boot.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ exports.forwardHttp = function (next) {
response.end();
});

forwarder.listen(ports.http , () => {
forwarder.listen(ports.http, () => {
log('[ok] forwarding http:// → https://');
next();
});
Expand Down Expand Up @@ -263,7 +263,7 @@ exports.ensureDockerTlsCertificates = function (next) {
return;
}

fs.chmod(path, 0600 /* read + write by owner */, (error) => {
fs.chmod(path, 0o600 /* read + write by owner */, (error) => {
if (error) {
log('[fail] unable to protect ' + path, error);
return;
Expand Down Expand Up @@ -292,6 +292,7 @@ exports.ensureDockerTlsCertificates = function (next) {
return;
}

// eslint-disable-next-line no-func-assign
done = null;
db.save();
log('[ok] new docker-tls credentials installed');
Expand Down Expand Up @@ -344,4 +345,3 @@ exports.registerDockerClient = function (next) {
next();
});
};

47 changes: 4 additions & 43 deletions lib/certificates.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
// Copyright © 2016 Jan Keromnes. All rights reserved.
// The following code is covered by the AGPL-3.0 license.

let child_process = require('child_process');
let forge = require('node-forge');
let letsencrypt = require('le-acme-core');
const child_process = require('child_process');
const forge = require('node-forge');
const letsencrypt = require('le-acme-core');

// ACME protocol client to interact with the Let's Encrypt service.
let client = letsencrypt.ACME.create({
Expand All @@ -18,11 +18,9 @@ let letsEncryptChallengePrefix =
// ACME protocol challenge tokens proving our identity to Let's Encrypt.
let letsEncryptChallenges = {};


// Verify if a given certificate in PEM format satisfies validity constraints.

exports.isValid = function (parameters, enableDebug) {

let debug = () => {
if (enableDebug) {
console.error(...arguments);
Expand Down Expand Up @@ -101,17 +99,13 @@ exports.isValid = function (parameters, enableDebug) {
}

return true;

};


// Generate an RSA public and private key pair in forge format (binary).

exports.generateRSAKeyPair = function (callback) {

// Generate a new 4096-bit RSA private key (up to 100x faster than forge).
child_process.exec('openssl genrsa 4096', (error, stdout, stderr) => {

if (error) {
callback(error);
return;
Expand All @@ -127,18 +121,13 @@ exports.generateRSAKeyPair = function (callback) {
privateKey: privateKey,
publicKey: publicKey
});

});

};


// Generate an SSH public and private key pair in OpenSSH format.

exports.createSSHKeyPair = function (callback) {

exports.generateRSAKeyPair((error, keypair) => {

if (error) {
callback(error);
return;
Expand All @@ -156,16 +145,12 @@ exports.createSSHKeyPair = function (callback) {
};

callback(null, sshKeyPair);

});

};


// Create a TLS certificate in PEM format.

exports.createTLSCertificate = function (parameters, callback) {

let commonName = parameters.commonName;
let altNames = parameters.altNames || [];
let basicConstraints = parameters.basicConstraints || null;
Expand Down Expand Up @@ -245,33 +230,26 @@ exports.createTLSCertificate = function (parameters, callback) {
certificate.setExtensions(extensions);

if (key) {

// A private key was provided, use it to finalize the certificate.
let privateKey = forge.pki.privateKeyFromPem(key);
let publicKey = forge.pki.setRsaPublicKey(privateKey.n, privateKey.e);
signOff({
privateKey: privateKey,
publicKey: publicKey
});
return;

} else {

// No private key was provided, generate a new one for this certificate.
exports.generateRSAKeyPair((error, keypair) => {
if (error) {
callback(error);
return;
}
signOff(keypair);
return;
});

}

// Use a given TLS key pair to sign and return the certificate.
function signOff (keypair) {

certificate.publicKey = keypair.publicKey;

if (caKey) {
Expand All @@ -289,17 +267,12 @@ exports.createTLSCertificate = function (parameters, callback) {
}

callback(null, crt, key);
return;

}

};


// Generate an HTTPS certificate using Let's Encrypt (https://letsencrypt.org/).

exports.createHTTPSCertificate = function (parameters, callback) {

let hostname = parameters.hostname;
let accountEmail = parameters.accountEmail;
let accountKey = parameters.accountKey || null;
Expand Down Expand Up @@ -345,7 +318,6 @@ exports.createHTTPSCertificate = function (parameters, callback) {

// Wait for all required tasks to finish before proceding.
function done () {

if (!acmeUrls || !accountKey || !httpsKey) {
// Some tasks are not finished yet. Let's wait.
return;
Expand All @@ -369,6 +341,7 @@ exports.createHTTPSCertificate = function (parameters, callback) {
}

// All required tasks are now finished, destroy the `done` callback.
// eslint-disable-next-line no-func-assign
done = null;

// We can now actually request an HTTPS certificate from Let's Encrypt.
Expand All @@ -384,21 +357,16 @@ exports.createHTTPSCertificate = function (parameters, callback) {
}
callback(null, certificate, accountKey);
});

}

};


// Expose the URL prefix that Let's Encrypt will use to challenge our identity.

exports.letsEncryptChallengePrefix = letsEncryptChallengePrefix;


// Look for an identity token that satisfies the given Let's Encrypt challenge.

exports.getLetsEncryptChallengeToken = function (url) {

if (!url || !url.startsWith(letsEncryptChallengePrefix)) {
// Not a Let's Encrypt challenge URL.
return null;
Expand All @@ -412,14 +380,11 @@ exports.getLetsEncryptChallengeToken = function (url) {
}

return token;

};


// Register a new Let's Encrypt account.

function registerLetsEncryptAccount (parameters, callback) {

let accountEmail = parameters.accountEmail;
let accountKey = parameters.accountKey;
let acmeUrls = parameters.acmeUrls;
Expand All @@ -437,14 +402,11 @@ function registerLetsEncryptAccount (parameters, callback) {
};

client.registerNewAccount(options, callback);

} // Don't export `registerLetsEncryptAccount`.


// Request HTTPS certificate issuance by Let's Encrypt via the ACME protocol.

function requestLetsEncryptCertificate (parameters, callback) {

let hostname = parameters.hostname;
let acmeUrls = parameters.acmeUrls;
let accountKey = parameters.accountKey;
Expand Down Expand Up @@ -473,5 +435,4 @@ function requestLetsEncryptCertificate (parameters, callback) {
};

client.getCertificate(options, callback);

Copy link
Member

Choose a reason for hiding this comment

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

Huge 👍! I used to do all these changes by hand... You're saving me so much time!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same here, just an auto-fix

} // Don't export `requestLetsEncryptCertificate`.
Loading