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

Add a spawn method for logging stdout and stderr. #2

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

chasingmaxwell
Copy link

This adds the spawn method so the stdout and stderr streams can be logged directly rather than being returned in a buffer. Example usage: drush.spawn(['cc', 'drush']);

@chasingmaxwell
Copy link
Author

I just realized drush_test.js and the readme should both be adjusted. Don't really have time for that tonight though :\

@chasingmaxwell
Copy link
Author

I was thinking about how to adjust the readme and I started wondering if, instead of having two commands, perhaps spawn should be used for the main exec command. Then whether or not output is logged during the command execution could be controlled by an option. What are your thoughts @elliotttf?

@@ -1,5 +1,7 @@
/* globals require, module, console */
Copy link
Owner

Choose a reason for hiding this comment

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

If this is a result of jshint stuff can you set node option in .jshintrc?

Copy link
Author

Choose a reason for hiding this comment

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

Hm, looks like the node option was already set. Might have been a hiccup in my syntastic config because I was seeing errors in vim and now I'm not. I'll remove it.

@elliotttf
Copy link
Owner

instead of having two commands, perhaps spawn should be used for the main exec command

I like the sound of that. Anything to reduce code repetition and increase simplicity is good by me!

Nice job on this. Admittedly it's been a loooong time since I've looked at the code so my comments above might be way off base. As long as the existing functionality isn't broken by this I'm happy to merge when you're ready.

@chasingmaxwell
Copy link
Author

I like the sound of that. Anything to reduce code repetition and increase simplicity is good by me!

Okay, I think I'll try to do that. There really isn't a reason I can tell that the two should be separate other than whether or not the output is logged automatically.

@chasingmaxwell
Copy link
Author

@elliotttf this is ready to go again. Drush.exec() now uses spawn instead of exec, but it still returns a promise for the output of the drush command. There was no longer any need for the maxBuffer option, but I added a log option to allow the logging of stdout and stderr on the spawned processes.

@patrickocoffeyo
Copy link
Collaborator

@chasingmaxwell I've also looked over the code, and it looks pretty great. @elliotttf I'll be functionally testing this soon and will report back.

drush.exec('updb')
.then(
function (res) {
console.log(res);
}
);

// Executes `drush cc all` and logs the output as the command is running.
drush.exec('cc all'], {log: true});
Copy link
Owner

Choose a reason for hiding this comment

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

s/\]//

Copy link
Author

Choose a reason for hiding this comment

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

wow, how did I not catch that. Sorry!

@elliotttf
Copy link
Owner

I think this looks good. Have you tried a database sync with these changes?

@chasingmaxwell
Copy link
Author

Have you tried a database sync with these changes?

@elliotttf I haven't tried sql-sync, but I have used the cat option to import a database with sql-cli and it worked just fine.

@elliotttf
Copy link
Owner

I'm specifically worried about the use case that drush-reloadp tackles. This was the driving force behind handling the large amount of output from the underlaying drush commands.

@chasingmaxwell
Copy link
Author

I've been pretty swamped the last couple days. If anyone else is able to test this with drush-reloadp that would be awesome (especially so aquifer can stop pointing to my fork of drush-node). Otherwise I'll get to this in the next few days.

@elliotttf
Copy link
Owner

Yeah, looks like it's not working, specifically with the way drush-reloadp does gzipping...

nbcunbc|create-media-collection ⇒ drush-reloadp -s @nbcunbc.acc -d @nbc.local
events.js:85
      throw er; // Unhandled 'error' event
            ^
Error: incorrect header check
    at Zlib._handle.onerror (zlib.js:366:17)

@chasingmaxwell
Copy link
Author

Bummer. Thanks for testing it! I'll look into it.

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.

4 participants