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

Add ability to start ember serve on https #3550

Merged
merged 6 commits into from May 20, 2015

Conversation

drogus
Copy link
Contributor

@drogus drogus commented Mar 16, 2015

Sometimes it may be needed to run an app locally with SSL. This PR
adds an --ssl option, which tries to run on https. Default ssl
certificate and ssl key paths are "ssl/server.crt" and "ssl/server.key"
respectively. Custom paths can be added with --ssl-cert and --ssl-key.

Tests don't pass, because I suck at node and npm and I have no idea how to run only
one file (and running all tests is too slow to debug it easily). Additionally I'm not sure
how to pass ssl options to subject in the test. They're used in setupHttpServer, but the test error says it's undefined. The weird thing is that ssl option apparently is passed through correctly, because it reaches a check for sslKey. Any help is appreciated.

@drogus drogus force-pushed the allow-to-serve-app-on-ssl branch 2 times, most recently from 8a07916 to f03d95d Compare March 16, 2015 20:16
@stefanpenner
Copy link
Contributor

I like the idea, i would like to play with it myself to see how it feels.
We would likely need to add some docs

@stefanpenner
Copy link
Contributor

@drogus got time to get tests green?

@stefanpenner
Copy link
Contributor

@drogus friendly ping

return subject.start({
host: '0.0.0.0',
port: '1337',
ssl: true
Copy link
Contributor

Choose a reason for hiding this comment

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

missing comma

@stefanpenner
Copy link
Contributor

Tests don't pass, because I suck at node and npm and I have no idea how to run only
one file (and running all tests is too slow to debug it easily).

select tests:

describe -> describe.only (now is selectively run)

debugging:

either set a debugger inline, or use the various debugger API's: https://nodejs.org/api/debugger.html

npm run test:debug

https://github.com/ember-cli/ember-cli/blob/master/package.json#L13

@stefanpenner
Copy link
Contributor

@drogus i hope this is enough info to help you make progress, let me know if i can help you further.

for some reason GH wouldn't let me PR to your fork, here is a branch i have rebased and made some fixes too: https://github.com/ember-cli/ember-cli/tree/pr-3550

pending work:

  • SSL cert is self-signed so the request test fails (as expected) But i wasn't sure your intent
  • some output seems to be incorrect

@drogus
Copy link
Contributor Author

drogus commented May 4, 2015

I'm sorry for a delay, my notifications setup was screwed up. I'll work on it today, thanks for tips, that should get me going!

@stefanpenner
Copy link
Contributor

I'm sorry for a delay, my notifications setup was screwed up. I'll work on it today, thanks for tips, that should get me going!

:)

@drogus drogus force-pushed the allow-to-serve-app-on-ssl branch from f03d95d to f7d4127 Compare May 5, 2015 07:55
})
.then(function() {
return new Promise(function(resolve, reject) {
process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stefanpenner the intention of this test was to check if we really start server over https, so I wanted to make a request that succeeds. I found out that the easiest way is to set process.env.NODE_TLS_REJECT_UNAUTHORIZED to 0, to not get an error without setting ca.

@drogus drogus force-pushed the allow-to-serve-app-on-ssl branch 2 times, most recently from 45bbc69 to 188c331 Compare May 5, 2015 09:58
@drogus
Copy link
Contributor Author

drogus commented May 5, 2015

It should be green now. Io.js tests fail, but I see them failing also on other branchs/prs, so that's rather not caused by my code.

@stefanpenner
Copy link
Contributor

the io.js failure appears different then the rest. The rest are actually the same test, that just happens to be brittle on travis. I am unable to reproduce it locally, on linux or OS X. :(

Anyways, re-kicked travis

@drogus
Copy link
Contributor Author

drogus commented May 5, 2015

I see the same issue here: https://travis-ci.org/ember-cli/ember-cli/jobs/61259446#L1843-L1848

Anyways, I can have a look at that as well.

@stefanpenner
Copy link
Contributor

ya hmm.. 17 hours ago we got a green build: https://travis-ci.org/ember-cli/ember-cli/builds/61205147 which should be master. It just take a-few runs.

@stefanpenner
Copy link
Contributor

@jayphelps looks like you appveyor has hijacked ember-cli's ?

https://ci.appveyor.com/project/jayphelps/ember-cli/build/230

@stefanpenner
Copy link
Contributor

some strange interleavings are happening with appveyor. https://ci.appveyor.com/project/embercli/ember-cli/build/2643 seems to be green.

@drogus drogus force-pushed the allow-to-serve-app-on-ssl branch from 188c331 to 58a31f5 Compare May 5, 2015 13:48
@jayphelps
Copy link
Member

@stefanpenner I don't even

drogus and others added 3 commits May 6, 2015 11:21
Sometimes it may be needed to run an app locally with SSL. This commit
adds an --ssl option, which tries to run on https. Default ssl
certificate and ssl key paths are "ssl/server.crt" and "ssl/server.key"
respectively. Custom paths can be added with --ssl-cert and --ssl-key
@drogus drogus force-pushed the allow-to-serve-app-on-ssl branch from 9685a6c to 6eadb5e Compare May 6, 2015 09:21
@drogus
Copy link
Contributor Author

drogus commented May 6, 2015

@stefanpenner it seems that tests pass after rebasing to master. Should I squash commits? Does the code look OK?

this.httpServer = this.http.createServer(this.app);
if (this.startOptions.ssl) {
if(!fs.existsSync(this.startOptions.sslKey)) {
throw new TypeError('Ssl key couldn\'t be found in "' + this.startOptions.sslKey + '", please provide a path to an existing ssl key file with --ssl-key');
Copy link
Contributor

Choose a reason for hiding this comment

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

Ssl -> SSL

@stefanpenner
Copy link
Contributor

  • left some small nitpick comments
  • coveralls appears to think we need a bit more test coverage...
    i suspect testing: startSSLHttpServer will be sufficient

@drogus
Copy link
Contributor Author

drogus commented May 7, 2015

@stefanpenner I'm not sure what's going on with coveralls, but after my last push (I extracted https server creation and fixed typos) it's green

stefanpenner added a commit that referenced this pull request May 20, 2015
Add ability to start ember serve on https
@stefanpenner stefanpenner merged commit de3f335 into ember-cli:master May 20, 2015
@drogus drogus deleted the allow-to-serve-app-on-ssl branch May 15, 2020 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants