Support Spring Boot 2 #87

BGehrels · 2018-07-09T18:32:00Z

Upgrading to Spring Boot 2, which will be a new major release

Documented that 4.2.0 should be used, if one is still using Spring Boot 1
Spring Boot acutator got a big API rework in Spring Boot 2 so a lot of adaptions were neccessary here:
- Endpoints are not exposed via HTTP by default any more, has to be configured explicitly => documented
- Endpoint code is now technology independent and works out of the box with HTTP and JMX => removed the MvcEndpoint
- Because the endpoint id is automagically used in http paths and config property names and some parts of springs property handly code doesn't support underscores, we had to switch to dashes
The new spring boot version automatically collects all FlywayCallback Beans and adds them to the main Flyway instance. Since we want to isolate our flyway config, we had to make sure that our callbacks have a different class name.
The Spring Boot Flyway AutoConfiguration can be disabled. In this case no FlywayProperties will exist leading to an NPE in FlywayMigrator => Fixed
There seems to be no need any more to wrap the Actuator config in a @ManagementContextConfiguration
Added a separate test module that uses the k8s way to configure access tokens. It can later be copy'n'pasted to test multiple permutation of optional dependencies
Deleted the deprecated Snapshot event provider, so that we don't carry deprecated interfaces into a new major release

Fixes #81.

ePaul · 2018-07-10T17:55:09Z

README.md

@@ -40,7 +40,8 @@ You may of course always setup a fresh system with the newest version.

 ## Prerequisites

-This library was tested with Spring Boot 1.5.3.RELEASE and relies on an existing configured PostgreSQL DataSource.
+This library was tested with Spring Boot 2.0.3.RELEASE and relies on an existing configured PostgreSQL DataSource. If
+you are still using Spring Boot 1.x, please use version 4.2.0 ([Release Notes](https://github.com/zalando-nakadi/nakadi-producer-spring-boot-starter/releases/tag/4.2.0), [Documentation](https://github.com/zalando-nakadi/nakadi-producer-spring-boot-starter/blob/4.2.0/README.md)) versions of this library.


The approach of making Spring-Boot 2 a dependency means that providing new features for applications which are still using Spring-Boot 1.x will be difficult (need to work on a separate branch?) or not done at all. I'm a bit hesitant to do this, given that the majority of the host-apps known to us are still based on Spring-Boot 1, and we still have quite some items in the wishlist which don't look like they would require Spring-Boot 2.

As an alternative approach, could we consider creating a new artifact (e.g. nakadi-producer-spring-boot-2-starter) in parallel to the existing nakadi-producer-spring-boot-starter (which would still support Spring Boot 1)? Both could be using the common nakadi-producer base library.

ePaul · 2018-07-17T14:53:10Z

There seems to be no need any more to wrap the Actuator config in a @ManagementContextConfiguration

Does this fix #72 (for the spring-boot 2 branch)?

ePaul · 2018-07-17T14:58:16Z

README.md


-will trigger a snapshot for the event type `my.event-type`. The (optional) request body is a "filter specifier".
+will trigger a snapshot for the event type `my.event-type`. There is an optional request parameter called "filter" that


A bit below (line 243 currently) we are still talking about "filter specifier", this should be adapted.

ePaul · 2018-07-17T15:04:31Z

...di-producer-spring-boot-starter/src/main/java/org/zalando/nakadiproducer/FlywayMigrator.java

@@ -22,10 +29,9 @@
    private DataSource dataSource;

    @Autowired(required = false)
-    @NakadiProducerFlywayCallback
-    private FlywayCallback callback;
+    private List<NakadiProducerFlywayCallback> callback;


If this is a list, could we name it in plural (callbacks)?

ePaul · 2018-07-17T15:17:51Z

...di-producer-spring-boot-starter/src/main/java/org/zalando/nakadiproducer/FlywayMigrator.java

        flyway.setBaselineOnMigrate(true);
        flyway.setBaselineVersionAsString("2133546886.1.0");
        flyway.migrate();
    }
+
+    private static class FlywayCallbackAdaptor extends BaseFlywayCallback {


Could we use the more common spelling "Adapter"?
(Adapter vs. Adaptor: What’s the Difference? → none, Adapter pattern)

ePaul · 2018-07-17T15:29:23Z

...ring-boot-starter/src/main/java/org/zalando/nakadiproducer/NakadiProducerFlywayCallback.java


 /**
- * Qualifier annotation for a FlywayCallback to be injected in to nakadi-producer's Flyway instance.
+ * This is the main callback interface that should be implemented to get access to flyway lifecycle notifications.
+ * Simply add code to the callback method you are interested in having.


Could we use default implementations for those methods doing nothing, so a host app doesn't need to implement all of them, if not all are needed?

ePaul · 2018-07-17T15:34:22Z

...r/src/main/java/org/zalando/nakadiproducer/snapshots/impl/SnapshotEventCreationEndpoint.java

-    public void invoke(String eventType, String filter) {
-        snapshotCreationService.createSnapshotEvents(eventType, filter);
+    @WriteOperation
+    public void createFilteredSnapshotEvents(@Selector String arg0, @Nullable String filter) {


Nicer parameter name, please?

could do so, but because of some wild java compiler flag foo spring boot is doing here, it might than get quite itchy to get the test to run in the IDE. i guess i'll add a comment instead

ePaul · 2018-07-17T15:41:33Z

...ng-boot-starter/src/test/java/org/zalando/nakadiproducer/NakadiProducerFlywayCallbackIT.java

+    @Test
+    @DirtiesContext // Needed to make sure that flyway gets executed for each of the tests and Callbacks are called again
+    public void flywayConfigurationIsSetIfCallbackIsConfigurationAware() {
+        verify(configurationAwareNakadiProducerFlywayCallback).setFlywayConfiguration(any(FlywayConfiguration.class));


Can we (and do we want to) verify that this method is called before the actual callback methods?

ePaul · 2018-07-17T15:43:33Z

...boot-starter/src/test/java/org/zalando/nakadiproducer/NonNakadiProducerFlywayCallbackIT.java

+    public void ourOwnFlywayConfigurationStillWorksFineWhenSpringsFlywayAutoconfigIsDisabled() {
+        // Yes, this is redundant to the other test in here.
+        // We consider it important to document the requirement, so it is here nonetheless.
+        // This test does just enough to test it.


Maybe this could be reworded to "The test setup done by the class annotations do just enough to test it"?

ePaul · 2018-07-17T15:44:38Z

...src/test/java/org/zalando/nakadiproducer/snapshots/SnapshotEventGenerationWebEndpointIT.java

@@ -21,13 +21,13 @@
 @RunWith(SpringRunner.class)
 @SpringBootTest(
    webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT,
-    properties = { "management.security.enabled=false", "zalando.team.id:alpha-local-testing", "nakadi-producer.scheduled-transmission-enabled:false" },
+    properties = { "management.security.enabled=false", "zalando.team.id:alpha-local-testing", "nakadi-producer.scheduled-transmission-enabled:false", "management.endpoints.web.exposure.include:snapshot-event-creation" },


Can we get some line breaks here? I have to scroll in Github to read it all.

ePaul · 2018-07-17T15:45:25Z

...src/test/java/org/zalando/nakadiproducer/snapshots/SnapshotEventGenerationWebEndpointIT.java

    classes = { TestApplication.class, EmbeddedDataSourceConfig.class, SnapshotEventGenerationWebEndpointIT.Config.class }
 )
 public class SnapshotEventGenerationWebEndpointIT {

    private static final String MY_EVENT_TYPE = "my.event-type";
-    private static final String MY_REQUEST_BODY = "my request body";
+    private static final String filter = "myRequestBody";


Constants in upper case, please?

ePaul · 2018-07-17T15:49:35Z

...src/test/java/org/zalando/nakadiproducer/snapshots/SnapshotEventGenerationWebEndpointIT.java

-        .when().post("/snapshot_event_creation/" + MY_EVENT_TYPE)
-        .then().statusCode(200);
+                .contentType("application/json")
+        .when().post("/actuator/snapshot-event-creation/" + MY_EVENT_TYPE + "?filter=" + filter)


Ah, I somehow missed that this is not a body parameter anymore. This should certainly be called out in the PR comment, and the Release notes.

Was this an intentional change, or a side effect of the new actuator stuff?

the later. It can still be given in the request body, but the format has changed. Already documented the changed format in the Readme, just pushed an additional IT to make sure the Format stays stable and included the fact in my release notes draft

ePaul · 2018-07-17T15:51:04Z

README.md


-will trigger a snapshot for the event type `my.event-type`. The (optional) request body is a "filter specifier".
+will trigger a snapshot for the event type `my.event-type`. There is an optional request parameter called "filter" that


"request parameter" → "query parameter", just to make it clear what is happening.

ePaul · 2018-07-17T16:02:35Z

...r-starter-spring-boot-2-test/src/main/java/org/zalando/nakadiproducer/tests/Application.java

+    public SnapshotEventGenerator snapshotEventGenerator() {
+        return new SimpleSnapshotEventGenerator("eventtype", new BiFunction<Object, String, List<Snapshot>>() {
+            @Override
+            public List<Snapshot> apply(Object withIdGreaterThan, String filter) {


Those three lines can be shortened using Java 8's Lambda syntax:

return new SimpleSnapshotEventGenerator("eventtype", (Object withIdGreaterThan, String filter) -> {

(and one } at the end gets lost too). That was the reason to implement the SimpleSnapshotEventGenerator at all.

ePaul · 2018-07-17T16:05:04Z

...r-starter-spring-boot-2-test/src/main/java/org/zalando/nakadiproducer/tests/Application.java

+            }
+        });
+
+        // Todo: Test that some events arrive at a local nakadi mock


Not yet usable, but to get an idea: https://github.com/ePaul/nakadi-mock

Woow, cool, keep us updated on the progress!

ePaul · 2018-07-17T16:10:44Z

nakadi-producer-starter-spring-boot-2-test/src/main/resources/tokens/nakadi-token-secret

@@ -0,0 +1 @@
+my fake token


How is this file used?

Ah, in the integration test. Could this go into src/test/resources?

ePaul · 2018-07-18T11:45:49Z

👍

… startup. Let's see what comes next.

- Documented that 4.2.0 should be used, if one is still using Spring Boot 1 - Spring Boot acutator got a big API rework in Spring Boot 2 so a lot of adaptions were neccessary here: -- Endpoints are not exposed via HTTP by default any more, has to be configured explicitly => documented -- Endpoint code is now technology independent and works out of the box with HTTP and JMX => removed the MvcEndpoint -- Because the endpoint id is automagically used in http paths and config property names and some parts of springs property handly code doesn't support underscores, we had to switch to dashes - The new spring boot version automatically collects *all* FlywayCallback Beans and adds them to the main Flyway instance. Since we want to isolate our flyway config, we had to make sure that our callbacks have a different class name. - The Spring Boot Flyway AutoConfiguration can be disabled. In this case no FlywayProperties will exist leading to an NPE in FlywayMigrator => Fixed - There seems to be no need any more to wrap the Actuator config in a @ManagementContextConfiguration - Added a seperate test module that uses the k8s way to configure access tokens. It can later be copy'n'pasted to test multiple permutation of optional dependencies - Deleted the deprecated Snapshot event provider, so that we don't carry deprecated interfaces into a new major release

…o nicely and gives us 16 Major Releases time before the Spring Boot 1.x release train overtakes the Spring Boot 2 one

BGehrels · 2018-07-18T12:19:34Z

👍

ePaul · 2018-07-18T12:21:45Z

👍

innokenty · 2018-07-18T12:58:03Z

...boot-starter/src/test/java/org/zalando/nakadiproducer/NonNakadiProducerFlywayCallbackIT.java

+@RunWith(SpringRunner.class)
+@SpringBootTest(
+        webEnvironment = SpringBootTest.WebEnvironment.MOCK,
+        properties = { "zalando.team.id:alpha-local-testing", "nakadi-producer.scheduled-transmission-enabled:false", "spring.flyway.enabled:false"},


"alpha-local-testing"? 😃

That was inherited from the previous project (tarbela-producer-spring-boot-starter), I guess.
Not sure where this team ID is used, though.

Yeah, it's probably just a copy-paste mistake and it doesn't affect anything in fact.

ePaul reviewed Jul 10, 2018

View reviewed changes

ePaul changed the title ~~Spring boot 2 test~~ Support Spring Boot 2 Jul 10, 2018

BGehrels force-pushed the spring-boot-2-test branch from e62d2b1 to e74994e Compare July 12, 2018 15:01

zalando-nakadi deleted a comment Jul 12, 2018

BGehrels force-pushed the spring-boot-2-test branch from a83551f to e74994e Compare July 13, 2018 13:27

zalando-nakadi deleted a comment Jul 17, 2018

ePaul reviewed Jul 17, 2018

View reviewed changes

zalando-nakadi deleted a comment Jul 18, 2018

ePaul mentioned this pull request Jul 18, 2018

Support spring boot 2 #89

Closed

BGehrels added 3 commits July 18, 2018 14:04

Adding a spring boot 2 test. At the moment, it only tests successfull…

35f54ce

… startup. Let's see what comes next.

Switching to Version 20.0.0 since it matches the 2.0 of Spring boot s…

bcebbd2

…o nicely and gives us 16 Major Releases time before the Spring Boot 1.x release train overtakes the Spring Boot 2 one

BGehrels force-pushed the spring-boot-2-test branch from d5f3c58 to bcebbd2 Compare July 18, 2018 12:15

BGehrels merged commit 8064262 into master Jul 18, 2018

BGehrels deleted the spring-boot-2-test branch July 18, 2018 12:25

innokenty reviewed Jul 18, 2018

View reviewed changes

ePaul mentioned this pull request Aug 7, 2018

Submission to Nakadi does not work without @EnableScheduling #92

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Spring Boot 2 #87

Support Spring Boot 2 #87

BGehrels commented Jul 9, 2018 •

edited by ePaul

Loading

ePaul Jul 10, 2018

ePaul commented Jul 17, 2018 •

edited

Loading

ePaul Jul 17, 2018

ePaul Jul 17, 2018

ePaul Jul 17, 2018

ePaul Jul 17, 2018

ePaul Jul 18, 2018

ePaul Jul 17, 2018

BGehrels Jul 18, 2018

ePaul Jul 17, 2018

ePaul Jul 17, 2018

ePaul Jul 17, 2018

ePaul Jul 17, 2018

ePaul Jul 17, 2018

BGehrels Jul 18, 2018

ePaul Jul 17, 2018

ePaul Jul 17, 2018 •

edited

Loading

ePaul Jul 17, 2018

BGehrels Jul 18, 2018

ePaul Jul 17, 2018

ePaul Jul 17, 2018

ePaul commented Jul 18, 2018

BGehrels commented Jul 18, 2018

ePaul commented Jul 18, 2018

innokenty Jul 18, 2018

ePaul Jul 18, 2018

innokenty Jul 18, 2018


		will trigger a snapshot for the event type `my.event-type`. The (optional) request body is a "filter specifier".
		will trigger a snapshot for the event type `my.event-type`. There is an optional request parameter called "filter" that

Support Spring Boot 2 #87

Support Spring Boot 2 #87

Conversation

BGehrels commented Jul 9, 2018 • edited by ePaul Loading

Choose a reason for hiding this comment

ePaul commented Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ePaul Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ePaul commented Jul 18, 2018

BGehrels commented Jul 18, 2018

ePaul commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BGehrels commented Jul 9, 2018 •

edited by ePaul

Loading

ePaul commented Jul 17, 2018 •

edited

Loading

ePaul Jul 17, 2018 •

edited

Loading