Skip to content
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.

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

Removing internal usage of registerCustomKind #4579

Merged
merged 1 commit into from Dec 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
Expand Up @@ -29,8 +29,9 @@
* Fix #4516: added support for blocking delete operations using the withTimeout methods: op.withTimeout(1, TimeUnit.MINUTE).delete() - will wait for up to 1 minute for the resources to be fully deleted. This makes for a more concise replacement of the deletingExisting method.

#### _**Note**_: Breaking changes
* Fix #4515: files located at the root of jars named model.properties, e.g. core.properties, have been removed
* Fix #3923: removed KubernetesResourceMappingProvider - a META-INF/services/io.fabric8.kubernetes.api.model.KubernetesResource list of resources is used instead.
* Fix #4515: files located at the root of jars named model.properties, e.g. core.properties, have been removed
* Fix #4579: the implicit registration of resource and list types that happens when using the resource(class) methods has been removed. This makes the behavior of the client more predictable as that was an undocumented side-effect. If you expect to see instances of a custom type from an untyped api call - typically KubernetesClient.load, KubernetesClient.resourceList, KubernetesClient.resource(InputStream|String), then you must either create a META-INF/services/io.fabric8.kubernetes.api.model.KubernetesResource file (see above #3923), or make calls to KubernetesDeserializer.registerCustomKind - however since KubernetesDeserializer is an internal class that mechanism is not preferred.
* Fix #4597: remove the deprecated support for `javax.validation.constraints.NotNull` in the `crd-generator`, to mark a property as `required` it needs to be annotated with `io.fabric8.generator.annotation.Required`

### 6.2.0 (2022-10-20)
Expand Down
Expand Up @@ -15,8 +15,10 @@
*/
package io.fabric8.kubernetes.client.dsl.internal;

import com.fasterxml.jackson.core.type.TypeReference;
import io.fabric8.kubernetes.api.builder.TypedVisitor;
import io.fabric8.kubernetes.api.builder.Visitor;
import io.fabric8.kubernetes.api.model.DefaultKubernetesResourceList;
import io.fabric8.kubernetes.api.model.DeletionPropagation;
import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.api.model.KubernetesResource;
Expand Down Expand Up @@ -66,6 +68,7 @@
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.lang.reflect.Type;
import java.net.HttpURLConnection;
import java.net.MalformedURLException;
import java.net.URL;
Expand Down Expand Up @@ -390,7 +393,16 @@ public CompletableFuture<L> submitList(ListOptions listOptions) {
try {
URL fetchListUrl = fetchListUrl(getNamespacedUrl(), defaultListOptions(listOptions, null));
HttpRequest.Builder requestBuilder = httpClient.newHttpRequestBuilder().url(fetchListUrl);
CompletableFuture<L> futureAnswer = handleResponse(httpClient, requestBuilder, listType, getParameters());
Type refinedType = listType.equals(DefaultKubernetesResourceList.class)
? Serialization.jsonMapper().getTypeFactory().constructParametricType(listType, type)
: listType;
TypeReference<L> listTypeReference = new TypeReference<L>() {
@Override
public Type getType() {
return refinedType;
}
};
CompletableFuture<L> futureAnswer = handleResponse(httpClient, requestBuilder, listTypeReference, getParameters());
return futureAnswer.thenApply(l -> {
updateApiVersion(l);
return l;
Expand Down
Expand Up @@ -15,17 +15,14 @@
*/
package io.fabric8.kubernetes.client.dsl.internal;

import io.fabric8.kubernetes.api.model.GenericKubernetesResource;
import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.api.model.KubernetesResource;
import io.fabric8.kubernetes.api.model.KubernetesResourceList;
import io.fabric8.kubernetes.client.Client;
import io.fabric8.kubernetes.client.dsl.MixedOperation;
import io.fabric8.kubernetes.client.dsl.Resource;
import io.fabric8.kubernetes.client.dsl.base.ResourceDefinitionContext;
import io.fabric8.kubernetes.client.impl.BaseClient;
import io.fabric8.kubernetes.client.utils.Utils;
import io.fabric8.kubernetes.internal.KubernetesDeserializer;

import static io.fabric8.kubernetes.client.utils.KubernetesResourceUtil.inferListType;

Expand All @@ -50,27 +47,13 @@ public HasMetadataOperationsImpl(OperationContext context, ResourceDefinitionCon
.withPlural(rdc.getPlural()), type, listType != null ? listType : (Class) inferListType(type));

this.rdc = rdc;

if (!GenericKubernetesResource.class.isAssignableFrom(type)) {
// TODO: the static nature of these registrations is problematic,
// we should ensure that we aren't redefining an existing type
KubernetesDeserializer.registerCustomKind(apiVersion, kind(rdc), type);
if (KubernetesResource.class.isAssignableFrom(this.listType)) {
KubernetesDeserializer.registerCustomKind(this.listType.getSimpleName(),
(Class<? extends KubernetesResource>) this.listType);
}
}
}

@Override
public HasMetadataOperationsImpl<T, L> newInstance(OperationContext context) {
return new HasMetadataOperationsImpl<>(context, rdc, type, listType);
}

private String kind(ResourceDefinitionContext context) {
return context.getKind() != null ? context.getKind() : getKind();
}

@Override
public boolean isResourceNamespaced() {
return rdc.isNamespaceScoped();
Expand Down
Expand Up @@ -48,6 +48,7 @@
import java.io.IOException;
import java.io.InputStream;
import java.io.InterruptedIOException;
import java.lang.reflect.Type;
import java.net.HttpURLConnection;
import java.net.MalformedURLException;
import java.net.URL;
Expand Down Expand Up @@ -566,7 +567,12 @@ protected <T> T handleResponse(HttpRequest.Builder requestBuilder, Class<T> type
*/
private <T> T handleResponse(HttpRequest.Builder requestBuilder, Class<T> type, Map<String, String> parameters)
throws IOException {
return waitForResult(handleResponse(httpClient, requestBuilder, type, parameters));
return waitForResult(handleResponse(httpClient, requestBuilder, new TypeReference<T>() {
@Override
public Type getType() {
return type;
}
}, parameters));
}

/**
Expand All @@ -580,7 +586,8 @@ private <T> T handleResponse(HttpRequest.Builder requestBuilder, Class<T> type,
*
* @return Returns a de-serialized object as api server response of provided type.
*/
protected <T> CompletableFuture<T> handleResponse(HttpClient client, HttpRequest.Builder requestBuilder, Class<T> type,
protected <T> CompletableFuture<T> handleResponse(HttpClient client, HttpRequest.Builder requestBuilder,
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be easier to provide an overloaded method instead of duplicating the TypeReference inner-class instantiation in most of the calling methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like how many overloaded methods there are already in OperationSupport, so it didn't seem worth it as it would just combine 2 calls.

Copy link
Member

Choose a reason for hiding this comment

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

sounds good

TypeReference<T> type,
Map<String, String> parameters) {
VersionUsageUtils.log(this.resourceT, this.apiGroupVersion);
HttpRequest request = requestBuilder.build();
Expand All @@ -592,7 +599,7 @@ protected <T> CompletableFuture<T> handleResponse(HttpClient client, HttpRequest
return futureResponse.thenApply(response -> {
try {
assertResponseCode(request, response);
if (type != null) {
if (type != null && type.getType() != null) {
return Serialization.unmarshal(new ByteArrayInputStream(response.body()), type, parameters);
} else {
return null;
Expand Down
Expand Up @@ -15,8 +15,6 @@
*/
package io.fabric8.kubernetes.client.impl;

import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.type.TypeFactory;
import io.fabric8.kubernetes.api.model.APIGroup;
import io.fabric8.kubernetes.api.model.APIGroupBuilder;
import io.fabric8.kubernetes.api.model.APIResource;
Expand Down Expand Up @@ -386,9 +384,7 @@ public NamespaceableResource<HasMetadata> resource(InputStream is) {
*/
@Override
public MixedOperation<Binding, KubernetesResourceList<Binding>, Resource<Binding>> bindings() {
return resources(Binding.class,
(Class<KubernetesResourceList<Binding>>) TypeFactory.rawClass(new TypeReference<KubernetesResourceList<Binding>>() {
}.getType()));
return resources(Binding.class);
}

/**
Expand Down
Expand Up @@ -15,26 +15,17 @@
*/
package io.fabric8.kubernetes.client.dsl.internal;

import io.fabric8.kubernetes.api.model.DefaultKubernetesResourceList;
import io.fabric8.kubernetes.api.model.GenericKubernetesResource;
import io.fabric8.kubernetes.api.model.KubernetesResource;
import io.fabric8.kubernetes.api.model.apiextensions.v1beta1.CustomResourceDefinition;
import io.fabric8.kubernetes.client.CustomResource;
import io.fabric8.kubernetes.client.CustomResourceList;
import io.fabric8.kubernetes.client.dsl.MixedOperation;
import io.fabric8.kubernetes.client.dsl.Resource;
import io.fabric8.kubernetes.client.dsl.base.CustomResourceDefinitionContext;
import io.fabric8.kubernetes.client.dsl.base.ResourceDefinitionContext;
import io.fabric8.kubernetes.client.impl.KubernetesClientImpl;
import io.fabric8.kubernetes.client.utils.Serialization;
import io.fabric8.kubernetes.model.annotation.Group;
import io.fabric8.kubernetes.model.annotation.Version;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

import java.util.stream.Stream;

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertNotNull;
Expand All @@ -61,71 +52,11 @@ void shouldReturnGenericKubernetesResourceWhenNotRegistered() {
.hasFieldOrPropertyWithValue("apiVersion", "sample.fabric8.io/v1");
}

@DisplayName("HasMetadataOperationsImpl registers custom kind")
@ParameterizedTest(name = "{index}: {1}")
@MethodSource("registerCustomKindInput")
void hasMetadataOperationsImplRegistersCustomKind(
String description,
ResourceDefinitionContext resourceDefinitionContext,
Class<? extends CustomResource<?, ?>> resourceClazz,
Class<? extends CustomResourceList<?>> resourceListClazz) {
// Given
new HasMetadataOperationsImpl(
new OperationContext(),
resourceDefinitionContext,
resourceClazz,
resourceListClazz);
// When
final KubernetesResource resource = Serialization.unmarshal("{\n" +
" \"apiVersion\": \"custom.group/v1alpha1\",\n" +
" \"kind\": \"MyCustomResource\"\n" +
"}");
// Then
assertThat(resource)
.isInstanceOf(MyCustomResource.class)
.hasFieldOrPropertyWithValue("apiVersion", "custom.group/v1alpha1");
}

static Stream<Arguments> registerCustomKindInput() {
final CustomResourceDefinition myCustomResourceCrd = CustomResourceDefinitionContext
.v1beta1CRDFromCustomResourceType(MyCustomResource.class).build();
return Stream.of(
Arguments.arguments(
"with typed custom resource and list",
CustomResourceDefinitionContext.fromCrd(myCustomResourceCrd),
MyCustomResource.class,
MyCustomResourceList.class),
Arguments.arguments(
"with typed custom resource and generic list",
CustomResourceDefinitionContext.fromCrd(myCustomResourceCrd),
MyCustomResource.class,
CustomResourceList.class),
Arguments.arguments(
"with manual ResourceDefinitionContext",
new ResourceDefinitionContext.Builder()
.withGroup("custom.group")
.withVersion("v1alpha1")
.withPlural("mycustomresources")
.build(),
MyCustomResource.class,
MyCustomResourceList.class));
}

@Group(MyCustomResource.GROUP)
@Version(MyCustomResource.VERSION)
public static class MyCustomResource extends CustomResource<Object, Object> {
public static final String GROUP = "custom.group";
public static final String VERSION = "v1alpha1";
}

public static class MyCustomResourceList extends CustomResourceList<MyCustomResource> {
}

@Group("sample.fabric8.io")
@Version("v1")
public static class Bar extends CustomResource<Object, Object> {
}

public static class BarList extends CustomResourceList<Bar> {
public static class BarList extends DefaultKubernetesResourceList<Bar> {
}
}
Expand Up @@ -15,10 +15,10 @@
*/
package io.fabric8.kubernetes.client.mock;

import io.fabric8.kubernetes.api.model.DefaultKubernetesResourceList;
import io.fabric8.kubernetes.api.model.DeletionPropagation;
import io.fabric8.kubernetes.api.model.KubernetesResourceList;
import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
import io.fabric8.kubernetes.client.CustomResourceList;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.fabric8.kubernetes.client.dsl.MixedOperation;
import io.fabric8.kubernetes.client.dsl.Resource;
Expand Down Expand Up @@ -58,8 +58,8 @@ void create() {

@Test
void list() {
KubernetesResourceList<Star> starList = new CustomResourceList<>();
((CustomResourceList<Star>) starList).setItems(Collections.singletonList(getStar()));
KubernetesResourceList<Star> starList = new DefaultKubernetesResourceList<>();
((DefaultKubernetesResourceList<Star>) starList).setItems(Collections.singletonList(getStar()));
server.expect().get().withPath("/apis/example.crd.com/v1alpha1/stars").andReturn(200, starList).once();
starClient = client.resources(Star.class);

Expand Down
Expand Up @@ -15,6 +15,7 @@
*/
package io.fabric8.openshift.client.dsl.internal.build;

import com.fasterxml.jackson.core.type.TypeReference;
import io.fabric8.kubernetes.api.model.Event;
import io.fabric8.kubernetes.api.model.EventList;
import io.fabric8.kubernetes.client.Client;
Expand Down Expand Up @@ -51,6 +52,7 @@
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.lang.reflect.Type;
import java.net.MalformedURLException;
import java.net.URL;
import java.util.List;
Expand Down Expand Up @@ -262,7 +264,12 @@ protected Build submitToApiServer(InputStream inputStream, long contentLength) {
.post("application/octet-stream", inputStream, contentLength)
.expectContinue()
.uri(getQueryParameters());
return waitForResult(handleResponse(newClient, requestBuilder, Build.class, null));
return waitForResult(handleResponse(newClient, requestBuilder, new TypeReference<Build>() {
@Override
public Type getType() {
return Build.class;
}
}, null));
} catch (Throwable e) {
// TODO: better determine which exception this should occur on
// otherwise we need to have the httpclient api open up to the notion
Expand Down