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
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
27 changes: 24 additions & 3 deletions src/tools/environment.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ const dotenv = require('dotenv');

const ensurePath = require('./ensure-path');

const { IS_TESTING } = require('../constants');

// Copy bundle environment into process.env, and vice versa,
// for convenience and compatibility with native environment vars.
const applyEnvironment = event => {
Expand All @@ -31,10 +33,29 @@ const cleanEnvironment = () => {
}
};

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

@eliangcs eliangcs Feb 28, 2018

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 && !IS_TESTING) {
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
38 changes: 30 additions & 8 deletions test/tools/environment.js
Original file line number Diff line number Diff line change
@@ -1,30 +1,52 @@
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', () => {
beforeEach(() => {
env = Object.assign({}, process.env);
});

it('should should only read .env', () => {
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');
done();
should.not.exist(process.env.CITY);
});

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

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

it('should parse a with filename, ignore defaults', () => {
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');
done();
should.not.exist(process.env.CITY);
should.not.exist(process.env.PIZZA);
});

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