Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

Fixing up SSL support #659

Merged
merged 1 commit into from
Jul 19, 2015
Merged

Conversation

lirantal
Copy link
Member

  1. On startup fallback to non-SSL mode if no cert/key is present to start in secure mode
  2. Consistency for the error messages printed out

@lirantal lirantal force-pushed the bugfix/secure-mode-fix branch from 62f1ff0 to 03b7f6f Compare July 17, 2015 08:26
@ilanbiala ilanbiala added this to the 0.4.0 milestone Jul 17, 2015
@@ -56,9 +56,9 @@ var validateEnvironmentVariable = function() {
console.log();
if (!environmentFiles.length) {
if (process.env.NODE_ENV) {
console.error(chalk.red('No configuration file found for "' + process.env.NODE_ENV + '" environment using development instead'));
console.error(chalk.red('+ Error: No configuration file found for "' + process.env.NODE_ENV + '" environment using development instead'));
Copy link
Member

Choose a reason for hiding this comment

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

Why is there a plus sign at the beginning?

Copy link
Member Author

Choose a reason for hiding this comment

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

To distinguish the rest of the startup text information on the screen from actual errors. I couldn't find any other better method for that. I can remove, it's just styling on the console.

@jloveland
Copy link
Contributor

LGTM

Should we have an option for the user to have the application exit out if the it could not be started with https? If I deliberately set config.secure = true, the thought is that I may never want my application to start using http. We could add a config called config.https_only = true. We could default it to false.

I understand if you all think this would be too opinionated or make the config too complicated, I just wanted to see what people think.

@lirantal
Copy link
Member Author

@jloveland I think wrapping too much logic around it is an overkill. If this is production or anything of real use case, the application will simply start with no SSL mode and accessing it via https:// will fail so it is straightforward and easy to understand that something went wrong there + there are the logs.

@jloveland
Copy link
Contributor

Agreed, thanks!

On Jul 18, 2015, at 9:22 AM, Liran Tal notifications@github.com wrote:

@jloveland I think wrapping too much logic around it is an overkill. If this is production or anything of real use case, the application will simply start with no SSL mode and accessing it via https:// will fail so it is straightforward and easy to understand that something went wrong there + there are the logs.


Reply to this email directly or view it on GitHub.

@lirantal
Copy link
Member Author

Thanks for the review guys, merging.

lirantal added a commit that referenced this pull request Jul 19, 2015
@lirantal lirantal merged commit 2f8e20e into meanjs:0.4.0 Jul 19, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants