Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(api): Move EventListener and dependent classes to echo-api, remove previous RestEventParser #790

Merged
merged 5 commits into from
Feb 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 3 additions & 9 deletions echo-api/README.md
Original file line number Diff line number Diff line change
@@ -1,12 +1,6 @@
# echo-api

A module for defining plugin extension interfaces.
This module provides API interfaces and models. In particular, it offers the following PF4J
`ExtensionPoint` interfaces for runtime extension:

WARNING: This is an Alpha module and is not stable. Consider this an experiment.

Currently there is one extension point `RestEventParser` which is limited to parsing events
in echo's `RestEventListener`. This is primarily for early iteration and testing purposes.

The more powerful extension point for Echo is likely to be `EchoEventListener` - this would enable a plugin
ecosystem for Echo and also allow us to break existing implementations of `EchoEventListener`
(echo-rest, echo-notifications, echo-telemetry, etc) up into discrete plugins.
- [EventListener](src/main/java/com/netflix/spinnaker/echo/api/events/EventListener.java)
7 changes: 7 additions & 0 deletions echo-api/echo-api.gradle
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
/*
* DO NOT ADD ANY ADDITIONAL NON-TEST DEPENDENCIES HERE!
*
* This module should be the only thing required by plugin implementors. In order to
* avoid dependency conflicts we should bring the bare minimum of transitive
* dependencies along for the ride -- ideally nothing besides kork-plugins-api.
*/
dependencies {
api "com.netflix.spinnaker.kork:kork-plugins-api"

Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
/*
* Copyright 2015 Netflix, Inc.
* Copyright 2020 Netflix, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
Expand All @@ -14,9 +14,8 @@
* limitations under the License.
*/

package com.netflix.spinnaker.echo.model;
package com.netflix.spinnaker.echo.api.events;

import com.fasterxml.jackson.annotation.JsonInclude;
import java.util.Map;
import java.util.UUID;
import lombok.Data;
Expand All @@ -25,11 +24,9 @@
@Data
public class Event {
public Metadata details;
public Map content;
public Map<String, Object> content;
public String rawContent;

@JsonInclude(JsonInclude.Include.NON_NULL)
public Map payload;
public Map<String, Object> payload;

public String eventId = UUID.randomUUID().toString();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there value in content and payload being typed? I'd really like to avoid bringing M<S, O> into the -api modules if possible. Perhaps for Echo, we can't really do anything about it until the other services sending events in are well-defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll merge as is and then explore how content and payload could be typed. There's a lot to unwind WRT how Event and TriggerEvent (and the various TriggerEvent types) deserialize the content field and I don't feel comfortable making any changes there yet.

Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright 2020 Netflix, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.netflix.spinnaker.echo.api.events;

import org.pf4j.ExtensionPoint;

/**
* This is the main touch point for Echo. Implementations of `EventListener` will, if wired up as a
* Spring bean, process all events that are posted into Echo.
*/
public interface EventListener extends ExtensionPoint {
jonsie marked this conversation as resolved.
Show resolved Hide resolved

/** Process an Echo {@link Event} */
void processEvent(Event event);
}
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
/*
* Copyright 2015 Netflix, Inc.
* Copyright 2020 Netflix, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
Expand All @@ -14,12 +14,15 @@
* limitations under the License.
*/

package com.netflix.spinnaker.echo.model;
package com.netflix.spinnaker.echo.api.events;

import java.util.Date;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import lombok.*;
import org.springframework.http.HttpHeaders;
import lombok.AllArgsConstructor;
import lombok.Data;
import lombok.NoArgsConstructor;

/** Represents event metadata */
@Data
Expand All @@ -33,6 +36,6 @@ public class Metadata {
private String project;
private String application;
private String _content_id;
private Map attributes;
private HttpHeaders requestHeaders = new HttpHeaders();
private Map<String, String> attributes;
private Map<String, List<String>> requestHeaders = new HashMap<>();
Copy link
Contributor

@luispollo luispollo Apr 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonsie It looks like this broke code that relied on the assumption of headers being converted to lowercase by HttpHeaders, as discussed in #840. I think it might be best to revert this line instead of fixing each specific downstream issue that comes up as a result, unless there was a good reason to convert this particular property to a Map.

Copy link
Contributor Author

@jonsie jonsie Apr 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, I didn't catch this. Unfortunately, we can't pull in the Spring dependency into echo-api as this module needs to be dependency free (basically, just Java), which is why I removed HttpHeaders.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might do the trick:

private Map<String, List<String>> requestHeaders = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

This file was deleted.

1 change: 1 addition & 0 deletions echo-core/echo-core.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/

dependencies {
api project(":echo-api")
implementation project(":echo-model")

api "com.netflix.spinnaker.kork:kork-plugins"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.netflix.spinnaker.echo.jackson.EchoObjectMapper;
import com.netflix.spinnaker.echo.model.Trigger;
import com.netflix.spinnaker.echo.model.trigger.BuildEvent;
import com.netflix.spinnaker.echo.services.IgorService;
Expand All @@ -39,7 +40,7 @@
public class BuildInfoService {
private final IgorService igorService;
private final RetrySupport retrySupport;
private final ObjectMapper objectMapper = new ObjectMapper();
private final ObjectMapper objectMapper = EchoObjectMapper.getInstance();

// Manual triggers try to replicate actual events (and in some cases build events) but rather than
// pass the event to
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
package com.netflix.spinnaker.echo.config;

import com.netflix.spinnaker.config.PluginsAutoConfiguration;
import com.netflix.spinnaker.echo.api.events.EventListener;
import com.netflix.spinnaker.echo.discovery.DiscoveryPollingConfiguration;
import com.netflix.spinnaker.echo.events.EchoEventListener;
import com.netflix.spinnaker.echo.events.EventPropagator;
import com.netflix.spinnaker.kork.PlatformComponents;
import com.netflix.spinnaker.kork.artifacts.parsing.DefaultJinjavaFactory;
Expand Down Expand Up @@ -55,7 +55,7 @@ public EchoCoreConfig(ApplicationContext context) {
@Bean
public EventPropagator propagator() {
EventPropagator instance = new EventPropagator();
for (EchoEventListener e : context.getBeansOfType(EchoEventListener.class).values()) {
for (EventListener e : context.getBeansOfType(EventListener.class).values()) {
instance.addListener(e);
}
return instance;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@

package com.netflix.spinnaker.echo.events;

import com.netflix.spinnaker.echo.model.Event;
import com.netflix.spinnaker.echo.api.events.Event;
import com.netflix.spinnaker.echo.api.events.EventListener;
import com.netflix.spinnaker.security.AuthenticatedRequest;
import java.util.ArrayList;
import java.util.List;
Expand All @@ -29,10 +30,10 @@
@Slf4j
@SuppressWarnings({"CatchException"})
public class EventPropagator {
private List<EchoEventListener> listeners = new ArrayList<>();
private List<EventListener> listeners = new ArrayList<>();
private Scheduler scheduler = Schedulers.io();

public void addListener(EchoEventListener listener) {
public void addListener(EventListener listener) {
listeners.add(listener);
log.info("Added listener " + listener.getClass().getSimpleName());
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
* Copyright 2020 Netflix, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.netflix.spinnaker.echo.jackson;

import static com.fasterxml.jackson.annotation.JsonInclude.Include.NON_NULL;
import static com.fasterxml.jackson.databind.DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES;

import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.datatype.jdk8.Jdk8Module;
import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
import com.netflix.spinnaker.echo.api.events.Event;
import com.netflix.spinnaker.echo.jackson.mixin.EventMixin;

public class EchoObjectMapper {
private EchoObjectMapper() {}

private static final ObjectMapper INSTANCE = newInstance();

public static ObjectMapper newInstance() {
return new ObjectMapper()
.addMixIn(Event.class, EventMixin.class)
.registerModule(new Jdk8Module())
.registerModule(new JavaTimeModule())
.disable(FAIL_ON_UNKNOWN_PROPERTIES)
.setSerializationInclusion(NON_NULL);
}

/**
* Return an ObjectMapper instance that can be reused. Do not change the configuration of this
* instance as it will be shared across the entire application, use {@link #newInstance()}
* instead.
*
* @return Reusable ObjectMapper instance
*/
public static ObjectMapper getInstance() {
return INSTANCE;
}
}
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
/*
* Copyright 2015 Netflix, Inc.
* Copyright 2020 Netflix, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
Expand All @@ -14,10 +14,15 @@
* limitations under the License.
*/

package com.netflix.spinnaker.echo.events;
package com.netflix.spinnaker.echo.jackson.mixin;

import com.netflix.spinnaker.echo.model.Event;
import static com.fasterxml.jackson.annotation.JsonInclude.Include.NON_NULL;

public interface EchoEventListener {
void processEvent(Event event);
import com.fasterxml.jackson.annotation.JsonInclude;
import java.util.Map;

public abstract class EventMixin {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EventMixin' 🎧


@JsonInclude(NON_NULL)
public Map<String, Object> payload;
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
*/
package com.netflix.spinnaker.echo.events

import com.netflix.spinnaker.echo.model.Event
import com.netflix.spinnaker.echo.api.events.EventListener
import com.netflix.spinnaker.echo.api.events.Event
import rx.schedulers.Schedulers
import spock.lang.Specification

Expand All @@ -29,8 +30,8 @@ class EventPropagationSpec extends Specification {
given:
EventPropagator propagator = new EventPropagator()
propagator.scheduler = Schedulers.immediate()
EchoEventListener l1 = Mock(EchoEventListener)
EchoEventListener l2 = Mock(EchoEventListener)
EventListener l1 = Mock(EventListener)
EventListener l2 = Mock(EventListener)

when:
propagator.addListener(l1)
Expand Down
Loading