Pre-pull request review

[nickgrealy]

@llaaiiqq has asked if we can provide some thoughts/insight/feedback before he submits a pull request:

I've extended it to listen to node online/offline events and job queue events. Code is on my fork. I haven't submitted a PR yet because I want to clean things up a bit first - adding nine more event types added a lot of boilerplate code, and I'd like to refactor things to trim it down.

I was wondering if you had any thoughts on where you'd like to see things go before I dive in.

Fork is here: https://github.com/llaaiiqq/groovy-events-listener-plugin/commits/master

@Jimilian - just wanted your input as well, as you've been large contributor to date.

[nickgrealy]

First of all, thanks for taking the time to contribute! It's much appreciated, and I'm sure it will help others.

Having a quick look at the code:

• I like the new functionality - (Node and Queue listeners make sense)
• I like the qualified event names (e.g. Queue.*, ,Run.*, etc)
• I like the processEvents method, how it cleans up some of the boiler plate code

Things I'd like to see:

• tests - I can't see any tests that cover the new functionality. This could be as simple as adding a few more scenarios in here. Let me know if you need a hand with writing/modifying cucumber acceptance tests.
• a screenshot - what do the new configuration options look like?
• documentation - can you update the following page to include the new event names and when they're triggered?

Things I need to add:

• static code analysis - basically I need to add this file into the project at some point. This will (hopefully) improve the quality of code submissions, and give people a guideline for code style (without being too oppressive??). I'll wait until after your pull-request if you prefer?
• configuration tests - have I covered enabling/disabling events?

[nickgrealy]

Screenshot... is length of the config widget going to be a problem here/are there any other display options?

Otherwise, looks good.

[Jimilian]

Code looks very good! Especially after refactoring for boilerplate code! @llaaiiqq, you did a great job!

Btw, can we add events in same interface as Jenkins does for "build steps"? Just list where you can pick up only one element.

[nickgrealy]

Btw, can we add events in same interface as Jenkins does for "build steps"?

@Jimilian - Do you mean using an "Add" button (like below), to add events that will be listened to? (If so, sounds great! Can you throw something together?)

[llaaiiqq]

Thanks for the feedback folks! PR incoming in a minute.

Regarding the config widget length - yeah, that going to get unmanageable in a hurry. Maybe just have all events always enabled and let the user pick and choose in their event handling script?

[Jimilian]

@nickgrealy Yes, I was talking about it, unfortunately, I'm not a Jelly expert. Probably, we can ask on Jenkins-dev.

@llaaiiqq It's a bad idea, because it will create huge overhead. Especially if synchronized is enabled.