Skip to content

fixes and api improvements #775

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 3, 2022
Merged

fixes and api improvements #775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 5 additions & 1 deletion ...ator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Context.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,9 @@ public interface Context {

Optional<RetryInfo> getRetryInfo();

<T> T getSecondaryResource(Class<T> expectedType, String... qualifier);
default <T> T getSecondaryResource(Class<T> expectedType) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@metacosm is this ok with you, kind would like it to limit it in this scope. Since more qualifiers does not make sense, and if dependent resources will use the previous way it will be backwards compatible anyway. So it can be changed back without breaking the api.

return getSecondaryResource(expectedType, null);
}

<T> T getSecondaryResource(Class<T> expectedType, String eventSourceName);
}
4 changes: 2 additions & 2 deletions ...amework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DefaultContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ public Optional<RetryInfo> getRetryInfo() {
}

@Override
public <T> T getSecondaryResource(Class<T> expectedType, String... qualifier) {
public <T> T getSecondaryResource(Class<T> 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);
}
}
39 changes: 22 additions & 17 deletions ...k-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,11 +140,16 @@ public ControllerResourceEventSource<R> getControllerResourceEventSource() {
}

public <S> Optional<ResourceEventSource<R, S>> getResourceEventSourceFor(
Class<S> dependentType, String... qualifier) {
Class<S> dependentType) {
return getResourceEventSourceFor(dependentType, null);
}

public <S> Optional<ResourceEventSource<R, S>> getResourceEventSourceFor(
Class<S> 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);
}
Expand Down Expand Up @@ -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);
Expand All @@ -210,7 +215,7 @@ private Class getDependentType(EventSource source) {
: source.getClass();
}

private String qualifier(EventSource source) {
private String name(EventSource source) {
return source.name();
}

Expand All @@ -233,7 +238,7 @@ private String keyFor(Class<?> dependentType) {
return key;
}

public <S> ResourceEventSource<R, S> get(Class<S> dependentType, String qualifier) {
public <S> ResourceEventSource<R, S> get(Class<S> dependentType, String name) {
final var sourcesForType = sources.get(keyFor(dependentType));
if (sourcesForType == null || sourcesForType.isEmpty()) {
return null;
Expand All @@ -244,13 +249,13 @@ public <S> ResourceEventSource<R, S> get(Class<S> 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;
Expand All @@ -259,29 +264,29 @@ public <S> ResourceEventSource<R, S> get(Class<S> 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<R, S>) 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<EventSource> findMatchingSource(String qualifier,
private Optional<EventSource> findMatchingSource(String name,
List<EventSource> 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();
}
}
Expand Down
3 changes: 1 addition & 2 deletions operator-framework-core/src/test/java/io/javaoperatorsdk/operator/ControllerManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -37,7 +37,6 @@ public void addingMultipleControllersForCustomResourcesWithDifferentVersionsShou
TestCustomResourceV2.class);

checkException(registered, duplicated);

}

private <T extends HasMetadata, U extends HasMetadata> void checkException(
Expand Down
23 changes: 13 additions & 10 deletions ...re/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourceManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -94,48 +95,50 @@ 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;

final var exception = assertThrows(OperatorException.class,
() -> 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
void retrievingAnEventSourceWhenMultipleAreRegisteredForATypeShouldRequireAQualifier() {
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
Expand Down
2 changes: 2 additions & 0 deletions ...work-core/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestCustomResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is needed to compare resources when we register a new reonciler. There was a tests for the registration of the same resource with multiple versions - what should fail. It did not fail without this, could not determine the kind. It was not failing until now because there were junit4 annotations used on test class

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

see ControllerManagerTest.java changes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If that functionality should work without this annotation, that should be a separate issues IMHO.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It shouldn't be needed here. Just for the v2.

public class TestCustomResource
extends CustomResource<TestCustomResourceSpec, TestCustomResourceStatus> {

Expand Down
2 changes: 2 additions & 0 deletions ...rk-core/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestCustomResourceV2.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I guess you added it to the V1 version for symmetry?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same as above

public class TestCustomResourceV2
extends CustomResource<TestCustomResourceSpec, TestCustomResourceStatus> {

Expand Down
2 changes: 1 addition & 1 deletion .../mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/MySQLSchemaReconciler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public List<EventSource> prepareEventSources(
public UpdateControl<MySQLSchema> 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);
Expand Down