Skip to content
This repository has been archived by the owner on Jul 12, 2019. It is now read-only.

read from .env by default in inject() #70

Merged
merged 3 commits into from
Feb 28, 2018
Merged

read from .env by default in inject() #70

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f40e031
read from .env by default in inject()
xavdid Feb 28, 2018
1a2aeca
Merge branch 'master' into read-from-env
xavdid Feb 28, 2018
fd59bb7
quieter tests
xavdid Feb 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
"dependencies": {
"bluebird": "3.5.0",
"content-disposition": "0.5.2",
"dotenv": "4.0.0",
"dotenv": "5.0.1",
"form-data": "2.2.0",
"lodash": "4.17.4",
"node-fetch": "1.7.1",
Expand Down
25 changes: 22 additions & 3 deletions src/tools/environment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,29 @@ const cleanEnvironment = () => {
}
};

const localFilepath = filename => {
return path.join(process.cwd(), filename || '');
Copy link
Member

@eliangcs eliangcs Feb 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure if it's valid use case: Could a user inject an environment file using an absolute path? For example:

const zapier = require('zapier-platform-core');
zapier.tools.env.inject('/absolute/path/to/environment/file');

If so, we may want to adjust a bit here because path.join doesn't seem to handle absolute paths:

> process.cwd()
'/Users/eliang/Projects/zapier-platform/core'
> path.join(process.cwd(), '/root')
'/Users/eliang/Projects/zapier-platform/core/root'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they couldn't, but they also couldn't before. In master, we resolved their filename to the local directory anyway. We could expand that, but that seems like a more disruptive change (instead of renaming, they have to pass the whole path in).

};

const injectEnvironmentFile = filename => {
filename = filename || '.environment';
const filepath = path.join(process.cwd(), filename);
dotenv.load({ path: filepath });
if (filename) {
filename = localFilepath(filename);
}
// reads ".env" if filename is falsy, needs full path otherwise
let result = dotenv.load({ path: filename });
if (result.error) {
// backwards compatibility
result = dotenv.load({ path: localFilepath('.environment') });
if (result.parsed) {
console.log(
[
'\nWARNING: `.environment` files will no longer be read by default in the next major version.',
'Either rename your file to `.env` or explicitly call this function with a filename:',
'\n zapier.tools.env.inject(".environment");\n\n'
].join('\n')
Copy link
Member

Choose a reason for hiding this comment

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

We may want to hide this when running tests.

);
}
}
};

module.exports = {
Expand Down
39 changes: 32 additions & 7 deletions test/tools/environment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,55 @@
require('should');
const should = require('should');
const mock = require('mock-fs');

const tools = require('../../src/tools/exported');
const fake_file = 'CITY="boulder"\nNAME="david"\nPIZZA="Blackjack"\n';
let env;

describe('read env', () => {
describe.only('read env', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Should we remove .only here? Seems mocha only runs the tests in test/tools/environment.js because of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh good catch. that was just for me, since I didn't want to run the whole suite every time. Will remove.

beforeEach(() => {
env = Object.assign({}, process.env);
});

it('should should only read .env', done => {
mock({
'.environment': fake_file,
'.environment': 'CITY="boulder"\nNAME="david"\n',
'.env': 'PIZZA="Blackjack"\n',
secrets: 'SECRET=very_secret_thing'
});
});

it('should parse a config', done => {
tools.env.inject();
process.env.PIZZA.should.equal('Blackjack');
should.not.exist(process.env.CITY);
done();
Copy link
Member

@eliangcs eliangcs Feb 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The tests are synchronous code, so I guess we don't need done() here. So are the other test cases in this file.

});

// this is temporary in v6, removed for v7
it('should read .environment if .env is missing', done => {
mock({
'.environment': 'CITY="boulder"\nNAME="david"\n'
});

tools.env.inject();
process.env.CITY.should.equal('boulder');
should.not.exist(process.env.PIZZA);
done();
});

it('should parse a with filename', done => {
it('should parse a with filename, ignore defaults', done => {
mock({
'.environment': 'CITY="boulder"\nNAME="david"\n',
'.env': 'PIZZA="Blackjack"\n',
secrets: 'SECRET=very_secret_thing'
});

tools.env.inject('secrets');
process.env.SECRET.should.equal('very_secret_thing');
should.not.exist(process.env.CITY);
should.not.exist(process.env.PIZZA);
done();
});

afterEach(() => {
mock.restore();
process.env = env;
});
});