From 0683d3d7789a48b77a8802b0774c58b44cffa8cf Mon Sep 17 00:00:00 2001 From: csviri Date: Thu, 23 Dec 2021 11:04:02 +0100 Subject: [PATCH] fix: API review, minor changes --- .../operator/api/reconciler/Context.java | 6 ++- .../api/reconciler/DefaultContext.java | 4 +- .../processing/event/EventSourceManager.java | 39 +++++++++++-------- .../operator/ControllerManagerTest.java | 3 +- .../event/EventSourceManagerTest.java | 23 ++++++----- .../sample/simple/TestCustomResource.java | 2 + .../sample/simple/TestCustomResourceV2.java | 2 + .../sample/MySQLSchemaReconciler.java | 2 +- 8 files changed, 48 insertions(+), 33 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Context.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Context.java index 4c716c2842..367de503d1 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Context.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Context.java @@ -6,5 +6,9 @@ public interface Context { Optional getRetryInfo(); - T getSecondaryResource(Class expectedType, String... qualifier); + default T getSecondaryResource(Class expectedType) { + return getSecondaryResource(expectedType, null); + } + + T getSecondaryResource(Class expectedType, String eventSourceName); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DefaultContext.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DefaultContext.java index 7aa856097e..8ec9f2c39b 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DefaultContext.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DefaultContext.java @@ -23,9 +23,9 @@ public Optional getRetryInfo() { } @Override - public T getSecondaryResource(Class expectedType, String... qualifier) { + public T getSecondaryResource(Class expectedType, String eventSourceName) { final var eventSource = - controller.getEventSourceManager().getResourceEventSourceFor(expectedType, qualifier); + controller.getEventSourceManager().getResourceEventSourceFor(expectedType, eventSourceName); return eventSource.map(es -> es.getAssociated(primaryResource)).orElse(null); } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceManager.java index fb62b8b22c..bdd7504d35 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceManager.java @@ -140,11 +140,16 @@ public ControllerResourceEventSource getControllerResourceEventSource() { } public Optional> getResourceEventSourceFor( - Class dependentType, String... qualifier) { + Class dependentType) { + return getResourceEventSourceFor(dependentType, null); + } + + public Optional> getResourceEventSourceFor( + Class dependentType, String qualifier) { if (dependentType == null) { return Optional.empty(); } - String name = (qualifier != null && qualifier.length >= 1) ? qualifier[0] : ""; + String name = qualifier == null ? "" : qualifier; final var eventSource = eventSources.get(dependentType, name); return Optional.ofNullable(eventSource); } @@ -192,13 +197,13 @@ public boolean contains(EventSource source) { if (eventSources == null || eventSources.isEmpty()) { return false; } - return findMatchingSource(qualifier(source), eventSources).isPresent(); + return findMatchingSource(name(source), eventSources).isPresent(); } public void add(EventSource eventSource) { if (contains(eventSource)) { throw new IllegalArgumentException("An event source is already registered for the " - + keyAsString(getDependentType(eventSource), qualifier(eventSource)) + + keyAsString(getDependentType(eventSource), name(eventSource)) + " class/name combination"); } sources.computeIfAbsent(keyFor(eventSource), k -> new ArrayList<>()).add(eventSource); @@ -210,7 +215,7 @@ private Class getDependentType(EventSource source) { : source.getClass(); } - private String qualifier(EventSource source) { + private String name(EventSource source) { return source.name(); } @@ -233,7 +238,7 @@ private String keyFor(Class dependentType) { return key; } - public ResourceEventSource get(Class dependentType, String qualifier) { + public ResourceEventSource get(Class dependentType, String name) { final var sourcesForType = sources.get(keyFor(dependentType)); if (sourcesForType == null || sourcesForType.isEmpty()) { return null; @@ -244,13 +249,13 @@ public ResourceEventSource get(Class dependentType, String qualifie if (size == 1) { source = sourcesForType.get(0); } else { - if (qualifier == null || qualifier.isBlank()) { + if (name == null || name.isBlank()) { throw new IllegalArgumentException("There are multiple EventSources registered for type " + dependentType.getCanonicalName() - + ", you need to provide a qualifier to specify which EventSource you want to query. Known qualifiers: " - + sourcesForType.stream().map(this::qualifier).collect(Collectors.joining(","))); + + ", you need to provide a name to specify which EventSource you want to query. Known names: " + + sourcesForType.stream().map(this::name).collect(Collectors.joining(","))); } - source = findMatchingSource(qualifier, sourcesForType).orElse(null); + source = findMatchingSource(name, sourcesForType).orElse(null); if (source == null) { return null; @@ -259,29 +264,29 @@ public ResourceEventSource get(Class dependentType, String qualifie if (!(source instanceof ResourceEventSource)) { throw new IllegalArgumentException(source + " associated with " - + keyAsString(dependentType, qualifier) + " is not a " + + keyAsString(dependentType, name) + " is not a " + ResourceEventSource.class.getSimpleName()); } final var res = (ResourceEventSource) source; final var resourceClass = res.getResourceClass(); if (!resourceClass.isAssignableFrom(dependentType)) { throw new IllegalArgumentException(source + " associated with " - + keyAsString(dependentType, qualifier) + + keyAsString(dependentType, name) + " is handling " + resourceClass.getName() + " resources but asked for " + dependentType.getName()); } return res; } - private Optional findMatchingSource(String qualifier, + private Optional findMatchingSource(String name, List sourcesForType) { - return sourcesForType.stream().filter(es -> qualifier(es).equals(qualifier)).findAny(); + return sourcesForType.stream().filter(es -> name(es).equals(name)).findAny(); } @SuppressWarnings("rawtypes") - private String keyAsString(Class dependentType, String qualifier) { - return qualifier != null && qualifier.length() > 0 - ? "(" + dependentType.getName() + ", " + qualifier + ")" + private String keyAsString(Class dependentType, String name) { + return name != null && name.length() > 0 + ? "(" + dependentType.getName() + ", " + name + ")" : dependentType.getName(); } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/ControllerManagerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/ControllerManagerTest.java index c24a6dbe3e..a1cfa3e82d 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/ControllerManagerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/ControllerManagerTest.java @@ -1,6 +1,6 @@ package io.javaoperatorsdk.operator; -import org.junit.Test; +import org.junit.jupiter.api.Test; import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.client.CustomResource; @@ -37,7 +37,6 @@ public void addingMultipleControllersForCustomResourcesWithDifferentVersionsShou TestCustomResourceV2.class); checkException(registered, duplicated); - } private void checkException( diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourceManagerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourceManagerTest.java index 0c848634d6..b3bf230640 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourceManagerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourceManagerTest.java @@ -17,6 +17,7 @@ import io.javaoperatorsdk.operator.processing.event.source.EventSource; import io.javaoperatorsdk.operator.processing.event.source.controller.ControllerResourceEventSource; import io.javaoperatorsdk.operator.processing.event.source.timer.TimerEventSource; +import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -94,16 +95,16 @@ void retrievingEventSourceForClassShouldWork() { } @Test - void shouldNotBePossibleToAddEventSourcesForSameTypeAndQualifier() { + void shouldNotBePossibleToAddEventSourcesForSameTypeAndName() { EventSourceManager manager = initManager(); CachingEventSource eventSource = mock(CachingEventSource.class); - when(eventSource.getResourceClass()).thenReturn(String.class); + when(eventSource.getResourceClass()).thenReturn(TestCustomResource.class); when(eventSource.name()).thenReturn("name1"); manager.registerEventSource(eventSource); eventSource = mock(CachingEventSource.class); - when(eventSource.getResourceClass()).thenReturn(String.class); + when(eventSource.getResourceClass()).thenReturn(TestCustomResource.class); when(eventSource.name()).thenReturn("name1"); final var source = eventSource; @@ -111,8 +112,8 @@ void shouldNotBePossibleToAddEventSourcesForSameTypeAndQualifier() { () -> manager.registerEventSource(source)); final var cause = exception.getCause(); assertTrue(cause instanceof IllegalArgumentException); - assertTrue(cause.getMessage().contains( - "An event source is already registered for the (java.lang.String, name1) class/name combination")); + assertThat(cause.getMessage()).contains( + "An event source is already registered for the (io.javaoperatorsdk.operator.sample.simple.TestCustomResource, name1) class/name combination"); } @Test @@ -120,22 +121,24 @@ void retrievingAnEventSourceWhenMultipleAreRegisteredForATypeShouldRequireAQuali EventSourceManager manager = initManager(); CachingEventSource eventSource = mock(CachingEventSource.class); - when(eventSource.getResourceClass()).thenReturn(String.class); + when(eventSource.getResourceClass()).thenReturn(TestCustomResource.class); when(eventSource.name()).thenReturn("name1"); manager.registerEventSource(eventSource); CachingEventSource eventSource2 = mock(CachingEventSource.class); - when(eventSource2.getResourceClass()).thenReturn(String.class); + when(eventSource2.getResourceClass()).thenReturn(TestCustomResource.class); when(eventSource2.name()).thenReturn("name2"); manager.registerEventSource(eventSource2); final var exception = assertThrows(IllegalArgumentException.class, - () -> manager.getResourceEventSourceFor(String.class)); + () -> manager.getResourceEventSourceFor(TestCustomResource.class)); assertTrue(exception.getMessage().contains("name1")); assertTrue(exception.getMessage().contains("name2")); - assertTrue(manager.getResourceEventSourceFor(String.class, "name2").get().equals(eventSource2)); - assertTrue(manager.getResourceEventSourceFor(String.class, "name1").get().equals(eventSource)); + assertTrue(manager.getResourceEventSourceFor(TestCustomResource.class, "name2").get() + .equals(eventSource2)); + assertTrue(manager.getResourceEventSourceFor(TestCustomResource.class, "name1").get() + .equals(eventSource)); } @Test diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestCustomResource.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestCustomResource.java index d01bd3c747..65649bff57 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestCustomResource.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestCustomResource.java @@ -2,10 +2,12 @@ import io.fabric8.kubernetes.client.CustomResource; import io.fabric8.kubernetes.model.annotation.Group; +import io.fabric8.kubernetes.model.annotation.Kind; import io.fabric8.kubernetes.model.annotation.Version; @Group("sample.javaoperatorsdk.io") @Version("v1") +@Kind("TestCustomResource") public class TestCustomResource extends CustomResource { diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestCustomResourceV2.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestCustomResourceV2.java index e02e359bcc..6a5385c0f9 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestCustomResourceV2.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestCustomResourceV2.java @@ -2,10 +2,12 @@ import io.fabric8.kubernetes.client.CustomResource; import io.fabric8.kubernetes.model.annotation.Group; +import io.fabric8.kubernetes.model.annotation.Kind; import io.fabric8.kubernetes.model.annotation.Version; @Group("sample.javaoperatorsdk.io") @Version("v2") +@Kind("TestCustomResource") public class TestCustomResourceV2 extends CustomResource { diff --git a/sample-operators/mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/MySQLSchemaReconciler.java b/sample-operators/mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/MySQLSchemaReconciler.java index ce60142dd1..ce9d062927 100644 --- a/sample-operators/mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/MySQLSchemaReconciler.java +++ b/sample-operators/mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/MySQLSchemaReconciler.java @@ -52,7 +52,7 @@ public List prepareEventSources( public UpdateControl reconcile(MySQLSchema schema, Context context) { var dbSchema = context.getSecondaryResource(Schema.class); try (Connection connection = getConnection()) { - if (dbSchema != null) { + if (dbSchema == null) { var schemaName = schema.getMetadata().getName(); String password = RandomStringUtils.randomAlphanumeric(16); String secretName = String.format(SECRET_FORMAT, schemaName);