Strider-CD / strider-simple-runner

Easy-to-configure in-process Runner implementation for Strider.
MIT License
3 stars 18 forks source link

Pass emitter through to core so that it can be used in plugins #23

Closed microadam closed 10 years ago

microadam commented 10 years ago

This is a dependency for https://github.com/Strider-CD/strider-runner-core/pull/1 to work correctly

niallo commented 10 years ago

Looks good - test case would be great to prevent regressions.

microadam commented 10 years ago

Test case added

niallo commented 10 years ago

Excellent work - thanks!

jaredly commented 10 years ago

wait no...this is a problem

jaredly commented 10 years ago

as far as I understand, the emitter from the webapp should not be passed through to the worker context - they are separate concerns, and should not be this tightly coupled.

microadam commented 10 years ago

Oh right I see.

I might have to rethink some of this then.

Any ideas on how to achieve the same thing? Essentially, within my plugin, in the worker, I need to listen to an event which signifies the build has completed and then emit an event which the webapp part of the plugin can listen for and then carry out some actions.

jaredly commented 10 years ago

Take a look at strider-webhooks.

All events on the worker eventemitter that start with "plugin." are proxied to the webapp eventemitter. So in the worker, you emit "plugin.email.success" and in the webapp, you listen for it and then send the emails.

jaredly commented 10 years ago

man I feel bad that I've gotten so little documentation done >.< finals will be over in a few weeks, and then all will be well ;)

microadam commented 10 years ago

Ah! I see how this should all work now.

I had originally based my plugin off of the webhook plugin, however until this pull request (https://github.com/Strider-CD/strider-runner-core/pull/1) the webhook plugin could not have been working as the listen function (https://github.com/Strider-CD/strider-webhooks/blob/master/worker.js#L10) would have never been called.

Talking of documentation, is the documentation on the extension-loader correct in terms of how the listen function on a worker should work? it states:

// Listen for events on the internal job emitter.
//   Look at strider-runner-core for an
//   enumeration of the events. Emit plugin.[pluginid].myevent to
//   communicate things up to the browser or to the webapp.
      listen: function (emitter) {
      },

i.e only an emitter is passed in, however on the webhook plugin, it looks as though you are expecting emitter and context (https://github.com/Strider-CD/strider-webhooks/blob/master/worker.js#L10). Am I right in assuming that it should be emitter and context? If so, I will update my strider-core-runner pull request accordingly. I will also update it so that it is not passing through the webapp emitter, but the worker io object (and effectively revert this current pull request to prevent the coupling).

It also looks as though the proxying of events does not currently work (e.g https://github.com/Strider-CD/strider-webhooks/blob/master/worker.js#L16) due to that fact that only 'plugin.' is used here: https://github.com/Strider-CD/strider-simple-runner/blob/master/lib/index.js#L176 I believe this would need to be 'plugin..*' for your suggestion, and the webhook plugin, to work correctly.

I will look into getting this all working and send a few more pull requests!

microadam commented 10 years ago

should really use preview mode, that should have been

due to the fact that only 'plugin.*' is used.... I believe this would need to be 'plugin.*.*'
jaredly commented 10 years ago

Oh indeed - I though .* was enough, but it looks like you're right. And yes, context is also passed through to the listen function.

microadam commented 10 years ago

Excellent. Thanks for the update. I will refactor everything to work as suggested and send some more pull requests later on

jaredly commented 10 years ago

Thanks for all the work you're doing!

On Thu, Dec 5, 2013 at 8:46 AM, microadam notifications@github.com wrote:

Excellent. Thanks for the update. I will refactor everything to work as suggested and send some more pull requests later on

— Reply to this email directly or view it on GitHubhttps://github.com/Strider-CD/strider-simple-runner/pull/23#issuecomment-29908558 .