Skip to content

fix: same as controller config for KubernetesDependentResource standalone #1222

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
May 18, 2022
Merged

fix: same as controller config for KubernetesDependentResource standalone #1222

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fa6bcb7
fix: same as controller config for KubernetesDependentResource standa…
csviri May 18, 2022
ef697f9
smell fix
csviri May 18, 2022
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
25 changes: 20 additions & 5 deletions ...javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import io.fabric8.kubernetes.client.dsl.NonNamespaceOperation;
import io.fabric8.kubernetes.client.dsl.Resource;
import io.javaoperatorsdk.operator.api.config.informer.InformerConfiguration;
import io.javaoperatorsdk.operator.api.reconciler.Constants;
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext;
import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.DependentResourceConfigurator;
Expand All @@ -35,6 +36,7 @@ public abstract class KubernetesDependentResource<R extends HasMetadata, P exten
private final Matcher<R, P> matcher;
private final ResourceUpdatePreProcessor<R> processor;
private final Class<R> resourceType;
private KubernetesDependentResourceConfig kubernetesDependentResourceConfig;

@SuppressWarnings("unchecked")
public KubernetesDependentResource(Class<R> resourceType) {
Expand All @@ -49,12 +51,17 @@ public KubernetesDependentResource(Class<R> resourceType) {

@Override
public void configureWith(KubernetesDependentResourceConfig config) {
configureWith(config.labelSelector(), config.namespaces(), !config.wereNamespacesConfigured());
this.kubernetesDependentResourceConfig = config;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we call configureWith(…) here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we dont, have the context here, so we don't know the controller namespaces. So it cannot be done at this time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So when is it done, then? The expectation with the configureWith methods is that once they're called, the dependent resource is set up. In this case, this isn't true anymore and someone calling this method would need to call one of the other versions at some later time…

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 comment below

}

@SuppressWarnings("unchecked")
private void configureWith(String labelSelector, Set<String> namespaces,
boolean inheritNamespacesOnChange) {
boolean inheritNamespacesOnChange, EventSourceContext<P> context) {

if (namespaces.equals(Constants.SAME_AS_CONTROLLER_NAMESPACES_SET)) {
namespaces = context.getControllerConfiguration().getNamespaces();
}

final SecondaryToPrimaryMapper<R> primaryResourcesRetriever =
(this instanceof SecondaryToPrimaryMapper) ? (SecondaryToPrimaryMapper<R>) this
: Mappers.fromOwnerReference();
Expand Down Expand Up @@ -136,9 +143,17 @@ protected NonNamespaceOperation<R, KubernetesResourceList<R>, Resource<R>> prepa

@Override
protected InformerEventSource<R, P> createEventSource(EventSourceContext<P> context) {
configureWith(null, context.getControllerConfiguration().getNamespaces(), true);
log.warn("Using default configuration for " + resourceType().getSimpleName()
+ " KubernetesDependentResource, call configureWith to provide configuration");
if (kubernetesDependentResourceConfig != null) {
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 it's done here, when the event source is created.

Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔 I saw that but wasn't convinced there wasn't a path where this wouldn't get called… But I guess createEventSource is always called so we should be good! Thanks!

configureWith(kubernetesDependentResourceConfig.labelSelector(),
kubernetesDependentResourceConfig.namespaces(),
!kubernetesDependentResourceConfig.wereNamespacesConfigured(), context);
} else {
configureWith(null, context.getControllerConfiguration().getNamespaces(),
true, context);
log.warn(
"Using default configuration for {} KubernetesDependentResource, call configureWith to provide configuration",
resourceType().getSimpleName());
}
return eventSource();
}

Expand Down