search

jenkinsci / groovy-events-listener-plugin

A Jenkins plugin, which executes groovy code when an event occurs.
https://plugins.jenkins.io/groovy-events-listener-plugin/
MIT License
36 stars 34 forks source link

Added Listener for Configuration Changes #28

Closed vrenjith closed 7 years ago

vrenjith commented 8 years ago

Changes proposed in this pull request:

@nickgrealy

nickgrealy commented 8 years ago

@Jimilian - are you happy to approve the changes?

Jimilian commented 8 years ago

@nickgrealy To be honest, nope. I hope that @vrenjith can say why do we need to call parent method. Even in case then this method is not doing anything (empty).

I very appreciate any PR, so, we can merge it and after that I will remove code that I don't like. But maybe @vrenjith knows something about these methods that I don't. Also tests are missed :(

nickgrealy commented 8 years ago

@Jimilian - no problem, I'll wait until those changes have been discussed (tests would be good!) and let you perform the merge. 👍

vrenjith commented 7 years ago

I haven't checked the implementation of ItemListener and to be on the safe side I assumed it is always better to call the parent. Hence retained the super calls. But if @Jimilian thinks that it is unnecessary, I can just remove them and update the PR

Jimilian commented 7 years ago

@vrenjith Yes, it would be perfect. As you can see in any other listener in this plugin (i.e., https://github.com/jenkinsci/groovy-events-listener-plugin/blob/master/src/main/groovy/org/jenkinsci/plugins/globalEventsPlugin/GlobalRunListener.java) we don't call parent function. Otherwise it would be broken Observer pattern.

Jimilian commented 7 years ago

@vrenjith I uncommented checking java version and merged you PR. Thanks a lot for passion and patience:)

Jimilian commented 7 years ago

@nickgrealy, could you release new version, please? We found a beautiful use case there this feature could rock :)

nickgrealy commented 7 years ago

@Jimilian yep sure! Give me 15 mins...

nickgrealy commented 7 years ago

@Jimilian - just checking, are you happy with the state of the current master branch?

There's been a lot of changes since the last release - dc5f51d

Including this line - https://github.com/jenkinsci/groovy-events-listener-plugin/blob/master/build.gradle#L82 - which re-introduces your issue - https://github.com/jenkinsci/groovy-events-listener-plugin/issues/21

Jimilian commented 7 years ago

@nickgrealy Oups, I forgot about https://github.com/jenkinsci/groovy-events-listener-plugin/blob/master/build.gradle#L82.