Skip to content

[BUGFIX] execSync compat issue #92#93

Merged
lukemelia merged 1 commit intoember-cli-deploy:masterfrom
joebartels:swap_sync_exec
Mar 11, 2015
Merged

[BUGFIX] execSync compat issue #92#93
lukemelia merged 1 commit intoember-cli-deploy:masterfrom
joebartels:swap_sync_exec

Conversation

@joebartels
Copy link
Copy Markdown
Contributor

branch is intended to resolve issues around child_process execSync compatibility among the nodejs versions and platforms it's installed on. sync-exec is a good candidate. Another possible sol'n is latest version of execSync as noted by @duizendnegen in #92

This PR uses sync-exec and was confirmed to resolve #92 and is a potential fix for #90

@lukemelia
Copy link
Copy Markdown
Contributor

@joebartels Thanks for working on this. In the test, it looks like what you are stubbing doesn't match the usage. I think this is the source of the failure? Can you fix?

@joebartels
Copy link
Copy Markdown
Contributor Author

@lukemelia thanks for the feedback. I've updated how the sha test builds a stub for sync-exec and refactored createTag to work with how I changed the test... not proud of doing it like this. The other alternative I considered was instead of hardcoding a sha1 value in the sha test, it could be created from the actual git rev-parse HEAD at test runtime. e.g.,

// sha-test.js
var syncExec = require('sync-exec');
...
var GIT_SHA = syncExec('git rev-parse HEAD').stdout;

then no sandboxing is required for sync-exec. Although since it wasn't done this way I decided against it. Open to any suggestions (or PRs) :)

@joebartels
Copy link
Copy Markdown
Contributor Author

or create a utility, utilities/sync-exec.js that exports an object with the function require('sync-exec') as a method, that way it can be stubbed nicely for testing: joebartels@fceeca3

@lukemelia
Copy link
Copy Markdown
Contributor

Left you a PR @joebartels

@lukemelia
Copy link
Copy Markdown
Contributor

@joebartels After you merge my PR, go ahead and squash it down to one commit with a descriptive message, and I'll merge it in the morning.

joebartels pushed a commit to joebartels/ember-cli-deploy that referenced this pull request Mar 11, 2015
Fixes ember-cli-deploy#90, Fixes ember-cli-deploy#93

Allow option to pass syncExec function to tagging adapter via constructor instead.
Fixes ember-cli-deploy#90, Fixes ember-cli-deploy#92

Allow option to pass syncExec function to tagging adapter via constructor instead.
@joebartels
Copy link
Copy Markdown
Contributor Author

👍 really appreciate the help @lukemelia

lukemelia added a commit that referenced this pull request Mar 11, 2015
@lukemelia lukemelia merged commit d23a582 into ember-cli-deploy:master Mar 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dependency execSync for win64 requires rebuilding with node-gyp

2 participants