default to production #711

Merged
merged 1 commit into from Aug 3, 2016

Projects

None yet

3 participants

@jmervine
Member

Addressing comments on #706 around production vs development env.

@XhmikosR

@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-711 Jul 29, 2016 Inactive
@XhmikosR
Member

I don't think this is needed, at all.

I mean, can't we just set the env var in make.js for start target to production? I tried your branch with node make start and I still get the unminified Jade output.

@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-711 Jul 29, 2016 Inactive
@jmervine
Member
jmervine commented Jul 29, 2016 edited

If you're still getting the minified html with this branch, then you must have NODE_ENV=development set. You can run this to see what's set...

root@c32a159137f8:/app/user# node -e 'console.dir(process.env);'                                
{ NODE_VERSION: '4.4.7',
  HOSTNAME: 'c32a159137f8',
  TERM: 'xterm',
  NPM_CONFIG_LOGLEVEL: 'info',
  PATH: '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin',
  PWD: '/app/user',
  SHLVL: '1',
  HOME: '/root',
  PORT: '3000',
  _: '/usr/local/bin/node' }

note: I use docker for all development, hence the "root" and "/app/user" stuffs...

That said, if I pass production to start js, you'll get https redirection and won't be able to hit the page.

@XhmikosR
Member

No, I'm getting the unminifed HTML even with node make start.

@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-711 Jul 29, 2016 Inactive
@jmervine
Member

Okay, yeah... oops... I forgot I was forcing unset NODE_ENV to development up top. I reverted the bulk of this and it should be working now.

I still need to force the https redirection off unless configured otherwise to ensure that it works locally, but otherwise...

@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-711 Jul 29, 2016 Inactive
@XhmikosR
Member

Now node make start is using minified HTML but so does npm run run.

I still believe it's a lot simpler to just set the env var in make.js for the start target...

@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-711 Jul 29, 2016 Inactive
@jmervine
Member

@XhmikosR how's this?

@XhmikosR XhmikosR commented on an outdated diff Jul 29, 2016
@@ -1,6 +1,11 @@
{
- "name":"bootstrap-cdn",
- "scripts":{},
- "env":{},
- "addons":[]
+ "name":"bootstrap-cdn",
@XhmikosR
XhmikosR Jul 29, 2016 Member

While at it, maybe add a space before :.

@XhmikosR
Member

Much better although I'm getting an error when running make stop

C:\Users\xmr\Desktop\bootstrap-cdn>node make start
warn:    --minUptime not set. Defaulting to: 1000ms
warn:    --spinSleepTime not set. Your script will exit if it does not stay up for at least 1000ms
info:    Forever processing file: app.js
warn:    --minUptime not set. Defaulting to: 1000ms
warn:    --spinSleepTime not set. Your script will exit if it does not stay up for at least 1000ms
info:    Forever processing file: app.js

C:\Users\xmr\Desktop\bootstrap-cdn>node make stop
error:   Forever cannot find process with id: app.js
C:\Users\xmr\Desktop\bootstrap-cdn\node_modules\shelljs\src\common.js:340
        throw e;
        ^

Error: exec: internal error
    at Object.error (C:\Users\xmr\Desktop\bootstrap-cdn\node_modules\shelljs\src\common.js:67:11)
    at _exec (C:\Users\xmr\Desktop\bootstrap-cdn\node_modules\shelljs\src\exec.js:266:12)
    at C:\Users\xmr\Desktop\bootstrap-cdn\node_modules\shelljs\src\common.js:275:23
    at assertExec (C:\Users\xmr\Desktop\bootstrap-cdn\make.js:26:16)
    at Function.target.stop (C:\Users\xmr\Desktop\bootstrap-cdn\make.js:70:9)
    at Object.global.target.(anonymous function) [as stop] (C:\Users\xmr\Desktop\bootstrap-cdn\node_modules\shelljs\make.js:36:40)
    at C:\Users\xmr\Desktop\bootstrap-cdn\node_modules\shelljs\make.js:48:27
    at Array.forEach (native)
    at null._onTimeout (C:\Users\xmr\Desktop\bootstrap-cdn\node_modules\shelljs\make.js:46:10)
    at Timer.listOnTimeout (timers.js:92:15)
@jmervine
Member

Is that consistent? I've seen that periodically. I kind of just want to get rid of forever all together since we don't use it in production.

@XhmikosR
Member

Yeah, I'm getting that with node make travis in the enforce-lf branch.

  1946 passing (1m)
  1 failing

  1) functional bootstrap https://maxcdn.bootstrapcdn.com/twitter-bootstrap/2.1.1/js/bootstrap.min.js has integrity:

      AssertionError: 'sha384-v2XCZ4x+SDQreGSr5HtkSOzDeIzE4K5aqWp3xcskLxq7Y5S76C4sqh/MEurBY1fz' == 'sha384-gYjuefvjBnMR9aV2JTKd0GeQnxj+mKMBirUgIH+rBsMaUe1iAC8UtOnqPrZV8s2V'
      + expected - actual

      -sha384-v2XCZ4x+SDQreGSr5HtkSOzDeIzE4K5aqWp3xcskLxq7Y5S76C4sqh/MEurBY1fz
      +sha384-gYjuefvjBnMR9aV2JTKd0GeQnxj+mKMBirUgIH+rBsMaUe1iAC8UtOnqPrZV8s2V

      at C:\Users\xmr\Desktop\bootstrap-cdn\tests\functional_test.js:59:16
      at request (C:\Users\xmr\Desktop\bootstrap-cdn\tests\functional_test.js:44:16)
      at assertSRI (C:\Users\xmr\Desktop\bootstrap-cdn\tests\functional_test.js:55:5)
      at Context.<anonymous> (C:\Users\xmr\Desktop\bootstrap-cdn\tests\functional_test.js:97:21)
      at callFnAsync (C:\Users\xmr\Desktop\bootstrap-cdn\node_modules\mocha\lib\runnable.js:349:8)
      at Test.Runnable.run (C:\Users\xmr\Desktop\bootstrap-cdn\node_modules\mocha\lib\runnable.js:301:7)
      at Runner.runTest (C:\Users\xmr\Desktop\bootstrap-cdn\node_modules\mocha\lib\runner.js:422:10)
      at C:\Users\xmr\Desktop\bootstrap-cdn\node_modules\mocha\lib\runner.js:528:12
      at next (C:\Users\xmr\Desktop\bootstrap-cdn\node_modules\mocha\lib\runner.js:342:14)
      at C:\Users\xmr\Desktop\bootstrap-cdn\node_modules\mocha\lib\runner.js:352:7
      at next (C:\Users\xmr\Desktop\bootstrap-cdn\node_modules\mocha\lib\runner.js:284:14)
      at Immediate._onImmediate (C:\Users\xmr\Desktop\bootstrap-cdn\node_modules\mocha\lib\runner.js:320:5)



C:\Users\xmr\Desktop\bootstrap-cdn\node_modules\shelljs\src\common.js:340
        throw e;
        ^

Error: exec: internal error
    at Object.error (C:\Users\xmr\Desktop\bootstrap-cdn\node_modules\shelljs\src\common.js:67:11)
    at _exec (C:\Users\xmr\Desktop\bootstrap-cdn\node_modules\shelljs\src\exec.js:266:12)
    at C:\Users\xmr\Desktop\bootstrap-cdn\node_modules\shelljs\src\common.js:275:23
    at assertExec (C:\Users\xmr\Desktop\bootstrap-cdn\make.js:22:16)
    at Function.target.suite (C:\Users\xmr\Desktop\bootstrap-cdn\make.js:34:9)
    at Object.global.target.(anonymous function) [as suite] (C:\Users\xmr\Deskto
p\bootstrap-cdn\node_modules\shelljs\make.js:36:40)
    at Function.target.travis (C:\Users\xmr\Desktop\bootstrap-cdn\make.js:71:16)

    at Object.global.target.(anonymous function) [as travis] (C:\Users\xmr\Desktop\bootstrap-cdn\node_modules\shelljs\make.js:36:40)
    at C:\Users\xmr\Desktop\bootstrap-cdn\node_modules\shelljs\make.js:48:27
    at Array.forEach (native)
@XhmikosR
Member

Err, scratch that, even on Travis, we get the error thrown.

@jmervine
Member

Right! Because we do use make start, we use it for starting a demonized version of the app during functional testing.

@jmervine
Member

I'll need to look at this. I'm thinking that it means the process is crashing before node make stop is being run. Which isn't good.

@XhmikosR
Member

Can this be due to a change in recent shelljs version?

@XhmikosR
Member

Confirmed it's definitely an issue with the updated shelljs. Switching back to ^0.5.3 doesn't throw the internal error anymore. There are many issues on their repo https://github.com/shelljs/shelljs/issues?utf8=%E2%9C%93&q=is%3Aissue%20internal%20error

@jdorfman
Member

@XhmikosR @jmervine this will be looked at next week. Have a good weekend everybody =)

@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-711 Jul 30, 2016 Inactive
@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-711 Jul 30, 2016 Inactive
@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-711 Jul 30, 2016 Inactive
@jmervine jmervine commented on an outdated diff Jul 30, 2016
@@ -114,7 +124,10 @@ var MOCHA_OPTS = ' --timeout 15000 --slow 500';
var res = exec(BOOTLINT + ' -d W013 ' + output);
echo('+ node make stop');
- target.stop();
+ try {
+ // it's okay if this fails
+ target.stop();
+ } catch (e) { }
@jmervine
jmervine Jul 30, 2016 Member

I know this feels hacky, and it kind of is, but honestly it's really doesn't matter if this pass or fails, it has no impact on the bootlint action.

@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-711 Jul 30, 2016 Inactive
@XhmikosR
Member
XhmikosR commented Aug 1, 2016

@jmervine: this can be simplified to this only. https://github.com/MaxCDN/bootstrap-cdn/compare/xmr-jmervine-default-production

I can force push your branch if you want.

@jmervine
Member
jmervine commented Aug 1, 2016

@XhmikosR I'll add the spacing to app.json, but I want to keep the try / catch. There's not reason that failing to stop the app should ever cause bootlint to fail.

@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-711 Aug 1, 2016 Inactive
@jmervine
Member
jmervine commented Aug 1, 2016

@jdorfman @XhmikosR looks like the travis integration stopped running here...

@jmervine
Member
jmervine commented Aug 1, 2016

Nevermind, there it goes...

@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-711 Aug 1, 2016 Inactive
@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-711 Aug 1, 2016 Inactive
@jmervine jmervine and 1 other commented on an outdated diff Aug 1, 2016
@@ -72,9 +72,7 @@ var MOCHA_OPTS = ' --timeout 15000 --slow 500';
};
target.tryStop = function() {
- try {
- exec(FOREVER + ' stop app.js');
- } catch (e) { }
+ exec(FOREVER + ' stop app.js || true');
@jmervine
jmervine Aug 1, 2016 Member

@XhmikosR I know this is super annoying, but I'd probably do something similar in bash.

@XhmikosR
XhmikosR Aug 2, 2016 Member

Yeah, it's an understandable workaround.

@jmervine
Member
jmervine commented Aug 2, 2016

@XhmikosR gonna merge unless you have any objections

@XhmikosR
Member
XhmikosR commented Aug 2, 2016

Only thing please fetch and rebase against develop.

I'd like to test it one more time probably tomorrow morning.

On Aug 2, 2016 20:50, "Joshua Mervine" notifications@github.com wrote:

@XhmikosR https://github.com/XhmikosR gonna merge unless you have any
objections


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#711 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAVVtWX5Ykhvi-jAHD_gYvqp-6qJAwcaks5qb4NZgaJpZM4JYZJw
.

@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-711 Aug 2, 2016 Inactive
@jmervine
Member
jmervine commented Aug 2, 2016

fetched, rebased, squashed

@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-711 Aug 2, 2016 Inactive
@XhmikosR XhmikosR commented on the diff Aug 3, 2016
package.json
@@ -33,7 +33,7 @@
"request": "^2.64.0",
"serve-favicon": "^2.3.0",
"serve-static": "^1.10.0",
- "shelljs": "^0.5.3",
+ "shelljs": "^0.7.2",
@XhmikosR
XhmikosR Aug 3, 2016 Member

This shouldn't be here.

@jmervine
jmervine Aug 3, 2016 Member

I fixed the error with ^0.7.2 on this branch.

@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-711 Aug 3, 2016 Inactive
@jmervine jmervine temporarily deployed to bootstrapcdn-dev-pr-711 Aug 3, 2016 Inactive
@jmervine jmervine defaulting to production mode
cleaning up failed exec handling

fixing stop error
9cdd7c6
@jmervine jmervine merged commit 57a7225 into develop Aug 3, 2016

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
security/snyk No known vulnerabilities
Details
@jmervine jmervine deleted the jmervine-default-production branch Aug 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment