adding ability to configure session.secret in local env config #927
Conversation
| @@ -16,6 +16,7 @@ module.exports = { | |||
| // Enable mongoose debug mode | |||
| debug: process.env.MONGODB_DEBUG || false | |||
| }, | |||
| sessionSecret: process.env.SESSION_SECRET || 'MEAN', | |||
There was a problem hiding this comment.
This isn't really required because it's already coming from default.js config
jloveland
force-pushed
the
local-session-secret
branch
from
a0a4426
to
2de1525
Compare
|
@lirantal, updated please let me know what you think |
|
LGTM. I also like the addition of |
|
LGTM |
|
LGTM. Thanks! :-D |
| @@ -88,6 +88,23 @@ var validateSecureMode = function (config) { | |||
| }; | |||
|
|
|||
| /** | |||
| * Validate Session Secret parameter is not set to default in production | |||
| */ | |||
| var validateSessionSecret = function (config) { | |||
|
Can we get some tests for this? |
|
@ilanbiala I will add some tests. |
jloveland
force-pushed
the
local-session-secret
branch
from
2de1525
to
55f2b08
Compare
|
@ilanbiala added tests |
| @@ -88,6 +88,28 @@ var validateSecureMode = function (config) { | |||
| }; | |||
|
|
|||
| /** | |||
| * Validate Session Secret parameter is not set to default in production | |||
| */ | |||
| var validateSessionSecret = function (config, testing) { | |||
There was a problem hiding this comment.
Can you explain why this is a variable and not a named function?
There was a problem hiding this comment.
@ilanbiala that entire file is handled with variable functions like that: https://github.com/jloveland/mean/blob/local-session-secret/config/config.js , the validateSessionSecret is not an exception.
There was a problem hiding this comment.
@lirantal why is an anonymous function assigned to a variable better than a named function?
There was a problem hiding this comment.
@ilanbiala I'm pretty sure that was a "standard" that a lot of projects picked up and ran with for Node. Currently the rest of the file and server side js files are this way, and for consistency should be kept this way until a refactor is done, which IMO is outside the scope of this PR
There was a problem hiding this comment.
@ilanbiala you're missing the point. I'm not saying it's better or worse, I'm saying that the entire config.js file (https://github.com/jloveland/mean/blob/local-session-secret/config/config.js) makes use of the same style functions, i.e:
var getGlobbedPaths = function (globPatterns, excludes) {}
var validateEnvironmentVariable = function () {}
var validateSecureMode = function (config) {}
var validateSessionSecret = function (config, testing) {}
var initGlobalConfigFolders = function (config, assets) {}
and so onI don't think that this PR or others should turn into code style PRs. If we want to change this we can opt for another PR to update to better code styles.
There was a problem hiding this comment.
@lirantal agreed, but let's add this to the list of things to fix up in the formatting issue you are going to create.
|
LGTM with the new tests |
|
@jloveland can you please rebase so we can resolve the conflicts and merge? |
jloveland
force-pushed
the
local-session-secret
branch
from
55f2b08
to
2eb0b09
Compare
|
this is ready to merge |
|
Coverage increased, great work @jloveland ! :) |
| it('should accept default session secret when running in test', function (done) { | ||
| var conf = { sessionSecret: 'MEAN' }; | ||
| config.utils.validateSessionSecret(conf, true).should.equal(true); | ||
| done(); |
There was a problem hiding this comment.
This should be return done(). However, it doesn't look like there is any asynchronous code being tested, so why are we using done?
There was a problem hiding this comment.
@lirantal @jloveland if this is indeed synchronous (which I think it is), then we should revert this PR and remove the callback because it's unnecessary.
There was a problem hiding this comment.
@jloveland can you open a new PR to address the return done() standard as Ilan suggested?
@ilanbiala I agree with the async observation although for the sake of consistency and later updates to the tests it's ok to keep the done callback. After all, the code is not incorrect, but rather just uses a callback.
adding ability to configure session.secret in local env config
|
@ilanbiala thanks for the review! @lirantal I can submit a new PR to fix the issue @ilanbiala pointed out. |
fixing #926