sonyxperiadev / gerrit-events

MIT License
47 stars 62 forks source link

EventFilter - Add capability to modify which events are interesting #87

Closed Christoffer-Cortes closed 5 years ago

Christoffer-Cortes commented 5 years ago

This adds a method that takes a list of events that are specifically interesting. Events that are not matched with the list are considered uninteresting.

https://github.com/jenkinsci/gerrit-trigger-plugin/pull/397

Christoffer-Cortes commented 5 years ago

Looks good. One thing though we could discuss is the default interesting setting. Should null really set all to false and not to default? If not then maybe a setInterstingDefault?

At the moment if no list is provided then all events are set to true which is currently the default for all event types. Should they perhaps keep a default value individually or globally and utilize the setInterstingDefault method?

rsandell commented 5 years ago

Ah yes. I see now :) The original plan was to have all events interesting or not in the library, but we then just implemented those that actually where interesting. So for now it should be fine to have them all as default true. But as you can see in the enum declarations each one can specify the default where they are defined. I had in the back of my head that some of them where declared with false. But since that's not the case setting all to 'true' should work for now. Maybe add a TODO somewhere that if a type is added with the default false in the future we should revisit that part. wdyt?

Christoffer-Cortes commented 5 years ago

Sounds good. I've added a TODO. I need to make changes in the PR related to this in gerrit-trigger to make it compatible with this thinking. I assumed there that all events I fetch are true but that means I need to assign them as either filter in or out depending on each events interesting value.

Christoffer-Cortes commented 5 years ago

While doing some more testing and looking at the code I realised that each gerrit server in gerrit-trigger works with the same enum and values. This means that I can't have the setting per server as I originally thought. If it's for the entire gerrit-trigger plugin though then I think I need to rethink how I'm actually supposed to set the enum when the server is down. This is a problem currently in my solution and will be even more so if I make a global setting for the filter. GerritConnection get instanced and is started in the same method so it doesn't seem reliable to use that object to set the enum since if I make it a global setting then a server or a server connection might not exist... Sorry for the long text but it seems I need to rethink a little on the filter solution.

Christoffer-Cortes commented 5 years ago

Ok, so after realizing my misstake the setEventFilter has actually been moved to gerrit-trigger. Since enums are static I can't modify it on a per server basis. The filter setting has to be global as well then so it makes no sense having it in GerritConnection since that would require the existence of a gerrit server connection.

rsandell commented 5 years ago

OK So this PR should be closed then?

Christoffer-Cortes commented 5 years ago

I still need the ability to change the booleans for each type but the PR only concerns one method now. =)