feat(notifications): add NotificationAgent extension point #962
Conversation
| @@ -30,8 +31,7 @@ abstract class AbstractEventNotificationAgent implements EventListener { | |||
| @Autowired | |||
| Front50Service front50Service | |||
|
|
|||
| @Autowired(required = false) | |||
| protected ObjectMapper mapper | |||
| protected ObjectMapper mapper = EchoObjectMapper.getInstance() | |||
There was a problem hiding this comment.
Out of curiosity, why was this change needed?
There was a problem hiding this comment.
I end up wiring up each ExtensionNotificationAgent manually in EchoCoreConfig (tricky to map an arbitrary number of NotificationAgent beans into ExtensionNotificationAgent beans). I was passing the mapper through, but noticed that as of #790, most of the object mappers in Echo come from EchoObjectMapper and figured I'd save myself the trouble.
As for the autowired Front50Service and spinnakerUrl... they actually aren't used.
There was a problem hiding this comment.
As for the autowired Front50Service and spinnakerUrl... they actually aren't used.
Should we remove?
| String application, | ||
| Event event, | ||
| Map<String, String> config, | ||
| String status); |
There was a problem hiding this comment.
Are there existing typed and constrained status values for pipelines and orchestrations rather than a String. I'm not too worried about enums when it is a platform concern.
There was a problem hiding this comment.
The method signature is coming from this abstract class: https://github.com/spinnaker/echo/blob/master/echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/notification/AbstractEventNotificationAgent.groovy#L139.
This change would become a lot larger if I refactored that method now, but I think it would be nice to come back and give more concrete types for a lot of these parameters.
There was a problem hiding this comment.
Despite reading the method docs, I'm still not clear on the boundary between notificationConfig and config.
| import com.netflix.spinnaker.echo.api.events.NotificationAgent | ||
| import org.pf4j.Extension | ||
|
|
||
| @Extension |
There was a problem hiding this comment.
This is just for testing right? Should it be in test rather than src? And maybe named something that denotes that it is for the test like TestNotificationAgentExtension?
There was a problem hiding this comment.
Just following the pattern for the other test extension points in this project (https://github.com/spinnaker/echo/tree/master/echo-plugins-test/src/main/kotlin/com/netflix/spinnaker/echo/plugins)
|
|
||
| /** A NotificationAgent handles user-configured pipeline notifications. */ | ||
| @Alpha | ||
| public interface NotificationAgent extends SpinnakerExtensionPoint { |
There was a problem hiding this comment.
Would like to see @Nonnull etc on parameters and return types (or @NonnullByDefault etc)
| String application, | ||
| Event event, | ||
| Map<String, String> config, | ||
| String status); |
There was a problem hiding this comment.
Despite reading the method docs, I'm still not clear on the boundary between notificationConfig and config.
| @@ -30,8 +31,7 @@ abstract class AbstractEventNotificationAgent implements EventListener { | |||
| @Autowired | |||
| Front50Service front50Service | |||
|
|
|||
| @Autowired(required = false) | |||
| protected ObjectMapper mapper | |||
| protected ObjectMapper mapper = EchoObjectMapper.getInstance() | |||
There was a problem hiding this comment.
As for the autowired Front50Service and spinnakerUrl... they actually aren't used.
Should we remove?
danielpeach
force-pushed
the
dp/notification-agent
branch
from
533b394
to
b4fa52f
Compare
Adds NotificationAgent extension point.