Allow loading multiple ENV files #77

joeyespo · 2015-10-26T21:00:08Z

slnode · 2015-10-26T21:00:10Z

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

rmg · 2015-10-26T21:01:50Z

@slnode ok to test

rmg · 2015-10-26T21:18:01Z

lib/envs.js

@@ -138,6 +139,20 @@ function loadEnvs(path) {
  return env;
 }

+function loadEnvs(path) {
+  if (path.indexOf(',') === -1) {


Is this needed? I think path.split(',') below will handle this case as a single item Array and "just work", won't it?

You're correct. Was keeping the existing behavior when there's no ',' for a slight performance gain. It's negligible though, so I can remove it if that's preferred.

rmg · 2015-10-26T21:26:58Z

@joeyespo thanks for the pull request!

Is there any chance you could update the help message to indicate the added support for a comma separated list? And address my comments, if you don't mind.

joeyespo · 2015-10-27T01:39:02Z

@rmg Sure, will do!

joeyespo · 2015-10-27T02:36:49Z

@rmg Updated in 85901d1.

joeyespo · 2015-10-27T02:38:41Z

@rmg Any idea what's up with these test failures? My initial commit passed, now there's a bunch of failed linting errors from what I could see. Looks like they might be from generated .js in test/.

rmg · 2015-10-27T03:29:10Z

It's the env tests, all the keys are now in a different order than the fixtures. Hm...

joeyespo · 2015-10-27T16:06:08Z

@rmg Closing and re-opening to see if a fresh PR will fix the tests.

joeyespo · 2015-10-27T16:08:32Z

@rmg #78 is the new PR. Looks like that didn't help though, they failed right away.

Anything you need me to do?

slnode · 2015-10-27T16:08:59Z

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

rmg · 2015-10-27T20:26:15Z

@slnode ok to test

rmg · 2015-10-27T20:26:46Z

@joeyespo do the tests pass for you locally when you run npm test?

joeyespo · 2015-10-27T21:54:53Z

@rmg Here's my npm test output:

node-foreman>npm run test

> foreman@1.4.1 test node-foreman
> tap test

total ................................................... 0/0

ok

rmg · 2015-10-27T22:21:42Z

@joeyespo I just merged #79 to hopefully help with the tests a little bit. Could you try rebasing your branch on the current master and seeing if the tests work any better for you?

joeyespo · 2015-10-27T22:46:10Z

@rmg Done.

rmg · 2015-10-27T22:51:20Z

@joeyespo do the tests give you anything more useful when you run them locally now? You'll need to do npm update to get the newer tap release.

joeyespo · 2015-10-28T03:30:25Z

@rmg Getting the following output now, both on master and on multiple-env-files. (I pulled latest and ran npm update first.)

bash-4.3$ npm run test

> foreman@1.4.1 pretest node-foreman
> jshint *.js lib/*.js

ERROR: Can't open *.js
ERROR: Can't open lib/*.js

> foreman@1.4.1 test node-foreman
> tap test/*.test.*

ok test/console-output.test.js .......................... 1/1
ok test/console-trim.test.js ............................ 1/1
ok test/envs-commented.test.js .......................... 1/1
ok test/envs-empty.test.js .............................. 1/1
ok test/envs-json.test.js ............................... 1/1
ok test/envs-quoted.test.js ............................. 1/1

assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: false == true
    at process.<anonymous> (node-foreman\test\run-once.test.js:29:12)
    at emitOne (events.js:77:13)
    at process.emit (events.js:169:7)
not ok test/run-once.test.js ............................ 0/1
    Command: "C:\Program Files\nodejs\node.exe run-once.test.js"
    TAP version 13
    not ok 1 test/run-once.test.js
      ---
        exit:    1
        stderr:  |
          assert.js:89
            throw new assert.AssertionError({
            ^
          AssertionError: false == true
              at process.<anonymous> (node-foreman\test\run-once.test.js:29:12)
              at emitOne (events.js:77:13)
              at process.emit (events.js:169:7)
        command: |
          "C:\Program Files\nodejs\node.exe run-once.test.js"
      ...

    1..1
    # tests 1
    # fail  1

total ................................................... 6/7

not ok

npm ERR! Windows_NT 6.1.7601
npm ERR! argv "C:\\Program Files\\nodejs\\node.exe" "C:\\Program Files\\nodejs\\node_modules\\npm\\bin\\npm-cli.js" "run" "test"
npm ERR! node v4.1.0
npm ERR! npm  v2.14.3
npm ERR! code ELIFECYCLE
npm ERR! foreman@1.4.1 test: `tap test/*.test.*`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the foreman@1.4.1 test script 'tap test/*.test.*'.
npm ERR! This is most likely a problem with the foreman package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     tap test/*.test.*
npm ERR! You can get their info via:
npm ERR!     npm owner ls foreman
npm ERR! There is likely additional logging output above.

npm ERR! Please include the following file with any support request:
npm ERR!     node-foreman\npm-debug.log

On Windows though, so I had to run within git's bash. I noticed that once starts a process in a cross-platform way, but the test doesn't. So changing $FILENAME to %FILENAME%" produces:

bash-4.3$ npm run test

> foreman@1.4.1 pretest node-foreman
> jshint *.js lib/*.js

ERROR: Can't open *.js
ERROR: Can't open lib/*.js

> foreman@1.4.1 test node-foreman
> tap test/*.test.*

ok test/console-output.test.js .......................... 1/1
ok test/console-trim.test.js ............................ 1/1
ok test/envs-commented.test.js .......................... 1/1
ok test/envs-empty.test.js .............................. 1/1
ok test/envs-json.test.js ............................... 1/1
ok test/envs-quoted.test.js ............................. 1/1
ok test/run-once.test.js ................................ 1/1
total ................................................... 7/7

ok

Looks like the tests are passing locally.

While I'm at it, want me to make run-once.test.js cross-platform (when run within bash) in a separate PR?

rmg · 2015-10-28T17:01:21Z

Ah, right.. Windows wouldn't recognize all the tests that are actually shell scripts. Might need to port those to node to make them more portable.

joeyespo · 2015-10-28T20:24:23Z

@rmg Let me know what else you need from me.

glennr · 2015-12-17T05:49:32Z

+1 for this feature

glennr · 2015-12-17T05:58:45Z

nevermind - I see this works if you supply the -e flag to the nf command directly.

e.g.

nf -e .env.test start -s

Nice one!

rmg reviewed Oct 26, 2015
View reviewed changes

joeyespo closed this Oct 27, 2015

joeyespo mentioned this pull request Oct 27, 2015

Multiple env files #78

Closed

joeyespo reopened this Oct 27, 2015

joeyespo added 3 commits October 27, 2015 18:45

Allow loading multiple ENV files.

e56f7e3

Functionalize.

b88513d

Update help message.

f295db8

joeyespo force-pushed the multiple-env-files branch from 85901d1 to f295db8 Compare October 27, 2015 22:45

rmg mentioned this pull request Apr 30, 2016

support loading of multiple env files #114

Merged

rmg closed this in a0c3cf7 Apr 30, 2016

joeyespo deleted the multiple-env-files branch April 30, 2016 17:58

rmg mentioned this pull request Apr 30, 2016

Support for multiple env files as in the original foreman #87

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow loading multiple ENV files #77

Allow loading multiple ENV files #77

joeyespo commented Oct 26, 2015

slnode commented Oct 26, 2015

rmg commented Oct 26, 2015

rmg Oct 26, 2015

joeyespo Oct 26, 2015

rmg commented Oct 26, 2015

joeyespo commented Oct 27, 2015

joeyespo commented Oct 27, 2015

joeyespo commented Oct 27, 2015

rmg commented Oct 27, 2015

joeyespo commented Oct 27, 2015

joeyespo commented Oct 27, 2015

slnode commented Oct 27, 2015

rmg commented Oct 27, 2015

rmg commented Oct 27, 2015

joeyespo commented Oct 27, 2015

rmg commented Oct 27, 2015

joeyespo commented Oct 27, 2015

rmg commented Oct 27, 2015

joeyespo commented Oct 28, 2015

rmg commented Oct 28, 2015

joeyespo commented Oct 28, 2015

glennr commented Dec 17, 2015

glennr commented Dec 17, 2015

Allow loading multiple ENV files #77

Allow loading multiple ENV files #77

Conversation

joeyespo commented Oct 26, 2015

slnode commented Oct 26, 2015

rmg commented Oct 26, 2015

rmg Oct 26, 2015

Choose a reason for hiding this comment

joeyespo Oct 26, 2015

Choose a reason for hiding this comment

rmg commented Oct 26, 2015

joeyespo commented Oct 27, 2015

joeyespo commented Oct 27, 2015

joeyespo commented Oct 27, 2015

rmg commented Oct 27, 2015

joeyespo commented Oct 27, 2015

joeyespo commented Oct 27, 2015

slnode commented Oct 27, 2015

rmg commented Oct 27, 2015

rmg commented Oct 27, 2015

joeyespo commented Oct 27, 2015

rmg commented Oct 27, 2015

joeyespo commented Oct 27, 2015

rmg commented Oct 27, 2015

joeyespo commented Oct 28, 2015

rmg commented Oct 28, 2015

joeyespo commented Oct 28, 2015

glennr commented Dec 17, 2015

glennr commented Dec 17, 2015