Factor BuildEvent into BuildEvent and GitEvent #73
Conversation
lwander
force-pushed
the
factor-multiple-build-events
branch
from
cef4674
to
42f66d1
Compare
|
|
||
| private Action1<GitEvent> triggerEachMatchFrom(final List<Pipeline> pipelines) { | ||
| return event -> { | ||
| Observable.from(pipelines) |
There was a problem hiding this comment.
feels like we can move some of this stuff into the superclass without having to repeat it for every provider type
There was a problem hiding this comment.
I thought the same thing, but GitEvent and BuildEvent don't inherit from Event, making it tricky. Do we need to keep Event around as is?
There was a problem hiding this comment.
I agree with separating the events into different types but is there a pressing reason not to have an event interface or something so that we can unify some of this.
There was a problem hiding this comment.
I agree, but do you know if we need to keep Event.content as a Map: https://github.com/spinnaker/echo/blob/master/echo-model/src/main/groovy/com/netflix/spinnaker/echo/model/Event.groovy?
(Outside of the Git/BuildEvent use case)
There was a problem hiding this comment.
otherwise we would need to map every possible field from orca
Would suggest adding a TriggerEvent class for orca-pipelinetriggers and model Git, Cron and BuildEvents off that since it is essentially a different module and we don't have to deal with orca events as triggers.
lwander
force-pushed
the
factor-multiple-build-events
branch
2 times, most recently
from
a6868ad
to
3a36508
Compare
|
@tomaslin PTAL |
|
defer to @robfletcher since he's the original author. |
| import com.fasterxml.jackson.annotation.JsonIgnoreProperties | ||
| import groovy.transform.Canonical | ||
|
|
||
| @Canonical |
There was a problem hiding this comment.
Does it have any implications that this isn't on the superclass?
There was a problem hiding this comment.
If I understand it correctly, it should be fine since we are never instantiating the superclass. But I'll defer to you on that.
There was a problem hiding this comment.
It means any fields on the superclass won't participate in equals, hashCode or toString but if that's not an issue here then it shouldn't matter.
There was a problem hiding this comment.
Ah, I thought it was used for generating constructors. None of those methods are used for the TriggerEvent subclasses, so it should be fine.
Ready to merge?
There was a problem hiding this comment.
Yep
@Canonical generates a constructor with a param for each field – not including those in the superclass and it generates equals / hashCode / toString based on the same fields.
|
Looks good. I have a couple of questions above but they're minor things. |
lwander
force-pushed
the
factor-multiple-build-events
branch
from
3a36508
to
35113db
Compare
Factor BuildEvent into BuildEvent and GitEvent
@tomaslin