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

Inherit env and disable noop piping for emulators:exec. #1340

Merged
merged 3 commits into from
May 29, 2019

Conversation

yuchenshi
Copy link
Member

Description

This change makes the script of firestore emulators:exec inherit the environment variables (e.g. PATH). This should make sure firestore emulators:exec "npm test" always uses the local npm as expected.

It also disables piping of stdout and stderr since we're not changing them in anyway. As a nice side benefit, most CLI tools will now detect stdout as a terminal and turn on nice coloring, etc.

Scenarios Tested

Manually: firestore emulators:exec "env" to make sure environment variables are inherited. (As opposed to before where it only has the firestore emulator env var.) Also, manually firestore emulators:exec "which node" now correctly shows my local nvm node.

Manually firestore emulators:exec "npm test" in quickstart-nodejs/firestore-emulator/typescript-quickstart to make sure mocha prints nice colored output, as opposed to no colors before.

@googlebot googlebot added the cla: yes Manual indication that this has passed CLA. label May 28, 2019
src/commands/emulators-exec.ts Show resolved Hide resolved
@samtstern
Copy link
Contributor

@yuchenshi thanks! Approved but don't forget the changelog.

@coveralls
Copy link

coveralls commented May 28, 2019

Coverage Status

Coverage remained the same at 61.446% when pulling f42ba6b on ys/emulators-exec-env-pipe into 6e9066f on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 61.446% when pulling 139836e on ys/emulators-exec-env-pipe into 6e9066f on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Manual indication that this has passed CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants