Skip to content

refactor: make shouldUseSSA work with types instead of instances + tests #2538

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 2 commits into from
Sep 24, 2024
Merged

refactor: make shouldUseSSA work with types instead of instances + tests #2538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1849f31
refactor: make shouldUseSSA work with types instead of instances + tests
metacosm Sep 24, 2024
ef5dc6e
fix: shouldUseSSA should also work with unmanaged configuration
metacosm Sep 24, 2024
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
42 changes: 33 additions & 9 deletions ...ework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,24 +367,48 @@ default boolean ssaBasedCreateUpdateMatchForDependentResources() {
* not.
*
* @param dependentResource the {@link KubernetesDependentResource} under consideration
* @return {@code true} if SSA should be used for
* @param <R> the dependent resource type
* @param <P> the primary resource type
* @return {@code true} if SSA should be used for
* @since 4.9.4
Copy link
Collaborator

Choose a reason for hiding this comment

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

I personally don't think that deprecating should mean removing of old JavaDoc but I don't know what's the custom in this project.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we have one but removing the javadoc makes it even more obvious that people should really be using the other one instead, imo… 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've removed the deprecation in the end.

*/
default <R extends HasMetadata, P extends HasMetadata> boolean shouldUseSSA(
KubernetesDependentResource<R, P> dependentResource) {
if (dependentResource instanceof ResourceUpdaterMatcher) {
return shouldUseSSA(dependentResource.getClass(), dependentResource.resourceType(),
dependentResource.configuration().orElse(null));
}

/**
* This is mostly useful as an integration point for downstream projects to be able to reuse the
* logic used to determine whether a given {@link KubernetesDependentResource} type should use SSA
* or not.
*
* @param dependentResourceType the {@link KubernetesDependentResource} type under consideration
* @param resourceType the resource type associated with the considered dependent resource type
* @return {@code true} if SSA should be used for specified dependent resource type, {@code false}
* otherwise
* @since 4.9.5
*/
@SuppressWarnings("rawtypes")
default boolean shouldUseSSA(Class<? extends KubernetesDependentResource> dependentResourceType,
Class<? extends HasMetadata> resourceType,
KubernetesDependentResourceConfig<? extends HasMetadata> config) {
if (ResourceUpdaterMatcher.class.isAssignableFrom(dependentResourceType)) {
return false;
}
Optional<Boolean> useSSAConfig = dependentResource.configuration()
.flatMap(KubernetesDependentResourceConfig::useSSA);
// don't use SSA for certain resources by default, only if explicitly overriden
if (useSSAConfig.isEmpty()
&& defaultNonSSAResources().contains(dependentResource.resourceType())) {
return false;
Boolean useSSAConfig = Optional.ofNullable(config)
.flatMap(KubernetesDependentResourceConfig::useSSA)
.orElse(null);
// don't use SSA for certain resources by default, only if explicitly overridden
if (useSSAConfig == null) {
if (defaultNonSSAResources().contains(resourceType)) {
return false;
} else {
return ssaBasedCreateUpdateMatchForDependentResources();
}
} else {
return useSSAConfig;
}
return useSSAConfig.orElse(ssaBasedCreateUpdateMatchForDependentResources());
}

/**
Expand Down
3 changes: 2 additions & 1 deletion ...javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,8 @@ protected void addMetadata(boolean forMatch, R actualResource, final R target, P

protected boolean useSSA(Context<P> context) {
if (useSSA == null) {
useSSA = context.getControllerConfiguration().getConfigurationService().shouldUseSSA(this);
useSSA = context.getControllerConfiguration().getConfigurationService()
.shouldUseSSA(getClass(), resourceType(), configuration().orElse(null));
}
return useSSA;
}
Expand Down
6 changes: 3 additions & 3 deletions operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMatchingIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@
import io.fabric8.kubernetes.client.dsl.base.PatchContext;
import io.fabric8.kubernetes.client.dsl.base.PatchType;
import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension;
import io.javaoperatorsdk.operator.sample.dependentssa.DependentSSACustomResource;
import io.javaoperatorsdk.operator.sample.dependentssa.DependentSSAReconciler;
import io.javaoperatorsdk.operator.sample.dependentssa.DependentSSASpec;
import io.javaoperatorsdk.operator.sample.dependentssa.DependnetSSACustomResource;
import io.javaoperatorsdk.operator.sample.dependentssa.SSAConfigMapDependent;

import static org.assertj.core.api.Assertions.assertThat;
Expand Down Expand Up @@ -86,8 +86,8 @@ void testMatchingAndUpdate() {
});
}

public DependnetSSACustomResource testResource() {
DependnetSSACustomResource resource = new DependnetSSACustomResource();
public DependentSSACustomResource testResource() {
DependentSSACustomResource resource = new DependentSSACustomResource();
resource.setMetadata(new ObjectMetaBuilder()
.withName(TEST_RESOURCE_NAME)
.build());
Expand Down
20 changes: 10 additions & 10 deletions operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@
import io.fabric8.kubernetes.client.KubernetesClientBuilder;
import io.fabric8.kubernetes.client.utils.KubernetesResourceUtil;
import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension;
import io.javaoperatorsdk.operator.sample.dependentssa.DependentSSACustomResource;
import io.javaoperatorsdk.operator.sample.dependentssa.DependentSSAReconciler;
import io.javaoperatorsdk.operator.sample.dependentssa.DependentSSASpec;
import io.javaoperatorsdk.operator.sample.dependentssa.DependnetSSACustomResource;
import io.javaoperatorsdk.operator.sample.dependentssa.SSAConfigMapDependent;

import static org.assertj.core.api.Assertions.assertThat;
Expand All @@ -33,7 +33,7 @@ class DependentSSAMigrationIT {
@BeforeEach
void setup(TestInfo testInfo) {
SSAConfigMapDependent.NUMBER_OF_UPDATES.set(0);
LocallyRunOperatorExtension.applyCrd(DependnetSSACustomResource.class, client);
LocallyRunOperatorExtension.applyCrd(DependentSSACustomResource.class, client);
testInfo.getTestMethod().ifPresent(method -> {
namespace = KubernetesResourceUtil.sanitizeName(method.getName());
cleanup();
Expand All @@ -53,7 +53,7 @@ void cleanup() {
@Test
void migratesFromLegacyToWorksAndBack() {
var legacyOperator = createOperator(client, true, null);
DependnetSSACustomResource testResource = reconcileWithLegacyOperator(legacyOperator);
DependentSSACustomResource testResource = reconcileWithLegacyOperator(legacyOperator);

var operator = createOperator(client, false, null);
testResource = reconcileWithNewApproach(testResource, operator);
Expand All @@ -66,7 +66,7 @@ void migratesFromLegacyToWorksAndBack() {
@Test
void usingDefaultFieldManagerDoesNotCreatesANewOneWithApplyOperation() {
var legacyOperator = createOperator(client, true, null);
DependnetSSACustomResource testResource = reconcileWithLegacyOperator(legacyOperator);
DependentSSACustomResource testResource = reconcileWithLegacyOperator(legacyOperator);

var operator = createOperator(client, false,
FABRIC8_CLIENT_DEFAULT_FIELD_MANAGER);
Expand All @@ -83,7 +83,7 @@ void usingDefaultFieldManagerDoesNotCreatesANewOneWithApplyOperation() {
}

private void reconcileAgainWithLegacy(Operator legacyOperator,
DependnetSSACustomResource testResource) {
DependentSSACustomResource testResource) {
legacyOperator.start();

testResource.getSpec().setValue(INITIAL_VALUE);
Expand All @@ -98,8 +98,8 @@ private void reconcileAgainWithLegacy(Operator legacyOperator,
legacyOperator.stop();
}

private DependnetSSACustomResource reconcileWithNewApproach(
DependnetSSACustomResource testResource, Operator operator) {
private DependentSSACustomResource reconcileWithNewApproach(
DependentSSACustomResource testResource, Operator operator) {
operator.start();

await().untilAsserted(() -> {
Expand All @@ -124,7 +124,7 @@ private ConfigMap getDependentConfigMap() {
return client.configMaps().inNamespace(namespace).withName(TEST_RESOURCE_NAME).get();
}

private DependnetSSACustomResource reconcileWithLegacyOperator(Operator legacyOperator) {
private DependentSSACustomResource reconcileWithLegacyOperator(Operator legacyOperator) {
legacyOperator.start();

var testResource = client.resource(testResource()).create();
Expand Down Expand Up @@ -155,8 +155,8 @@ private Operator createOperator(KubernetesClient client, boolean legacyDependent
return operator;
}

public DependnetSSACustomResource testResource() {
DependnetSSACustomResource resource = new DependnetSSACustomResource();
public DependentSSACustomResource testResource() {
DependentSSACustomResource resource = new DependentSSACustomResource();
resource.setMetadata(new ObjectMetaBuilder()
.withNamespace(namespace)
.withName(TEST_RESOURCE_NAME)
Expand Down
Loading
Loading