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

Type definition for KubernetesObjectApi.read() is incorrect #2128

Open
jportner opened this issue Dec 27, 2024 · 2 comments · May be fixed by #2129
Open

Type definition for KubernetesObjectApi.read() is incorrect #2128

jportner opened this issue Dec 27, 2024 · 2 comments · May be fixed by #2129

Comments

@jportner
Copy link

Describe the bug

When reading a Kubernetes object, the request must include the apiVersion, kind, and metadata fields for the object. If the object is found, the response will include the spec and status.

It appears the KubernetesObjectApi.read() type definition was accidentally changed in the 1.0.0 release, so the request and response object are the same.

Old type definition in 0.x:

javascript/src/object.ts

Lines 19 to 27 in 8d4a84c

type KubernetesObjectHeader<T extends KubernetesObject | KubernetesObject> = Pick<
T,
'apiVersion' | 'kind'
> & {
metadata: {
name: string;
namespace?: string;
};
};

javascript/src/object.ts

Lines 344 to 350 in 8d4a84c

public async read<T extends KubernetesObject | KubernetesObject>(
spec: KubernetesObjectHeader<T>,
pretty?: string,
exact?: boolean,
exportt?: boolean,
options: { headers: { [name: string]: string } } = { headers: {} },
): Promise<{ body: T; response: http.IncomingMessage }> {

The request payload is KubernetesObjectHeader<T> and the response is T.

New type definition in 1.0.0:

javascript/src/object.ts

Lines 280 to 286 in 27ba0e1

public async read<T extends KubernetesObject | KubernetesObject>(
spec: T,
pretty?: string,
exact?: boolean,
exportt?: boolean,
options?: Configuration,
): Promise<T> {

The request payload is T and the response is T.

Client Version

N/A

Server Version

N/A

To Reproduce

TS playground: https://www.typescriptlang.org/play/?#code/JYWwDg9gTgLgBAKjgQwM5wNYA50DMoQhwDkAAhgK4BGAplAHY0w2oD0AxgDbA30wC09CABMaxANwAoScD51cydjTgBxANIBRALLJ6yAOY1hAYToxguYO2TM4NAB7N6w9NlQA6NdTqNmqAPJUAFY07PAA3pJw0ShgwABqdKjAEPQAXCS+AO7QGLL67voYNO4prABuAIwSUTF5zhnEOnqGJmYWVjZiUjFwIEzIwjbIGZG9vXr9GagwUPk949GTLGCKNAD807PztdEAvgvRqGCho7vjwoTIsqhbc-T6ANoAuodwB+czNhSom3BjizgSlgHWszAAcsgpnAZvd9G9esDzJYwTQAMowb63GHbB4ImKXEDXegYrGjOCE4l3eY4sk4uHvF746IOOJQGgAFVANGpePOHz20nYqRmQO4vHgAF44IwspgcJ5vMZUpZ9AAKACU7iJxQAgnFjOK+Gq3IraAwmCxAiEwvrgBqpJJcBR6GEUvQ4OzBqYQSiumrlrz9AAaGVQlZrIMa-7nYX0UVIuDSrg8PjuL3CAA86m0ugMRh9yM6zAAfGqAYtkHFElBkqlGtlcvlCsVShAKtVg+devVhI1mvm2r7i2Iu4C+gMhpjyctQ8tjmt3vyHed2TAKAwgWYpIKgA

Results in an error:

Argument of type '{ apiVersion: "networking.gke.io/v1"; kind: "ManagedCertificate"; metadata: { name: string; namespace: string; }; }' is not assignable to parameter of type 'GKEManagedCertificate'.
Property 'spec' is missing in type '{ apiVersion: "networking.gke.io/v1"; kind: "ManagedCertificate"; metadata: { name: string; namespace: string; }; }' but required in type 'GKEManagedCertificate'.(2345)
input.tsx(10, 5): 'spec' is declared here.

image

Expected behavior

For any type, only the apiVersion, kind, and metadata fields should be required.

Example Code

import * as k8s from '@kubernetes/client-node';

interface GKEManagedCertificate extends k8s.KubernetesObject {
    apiVersion: 'networking.gke.io/v1';
    kind: 'ManagedCertificate';
    metadata: {
        name: string;
        namespace?: string;
    };
    spec: {
        domains: string[];
    };
    status?: {
        certificateName: string;
        certificateStatus: string;
        domainStatus: { domain: string; status: string }[];
        expireTime: string;
    };
}

const client = new k8s.KubeConfig().makeApiClient(k8s.KubernetesObjectApi);

function readCertificate(name: string, namespace: string) {
    const cert = client.read<GKEManagedCertificate>({
        apiVersion: 'networking.gke.io/v1',
        kind: 'ManagedCertificate',
        metadata: { name, namespace }
    });
    return cert;
}

Environment (please complete the following information):

  • OS: macOS
  • NodeJS Version: 20.17.0
  • Cloud runtime: N/A

Additional context

This only affects type checking, not runtime.

@jportner jportner linked a pull request Dec 27, 2024 that will close this issue
@brendandburns
Copy link
Contributor

Looks like we need to cherry-pick this PR into the main branch

https://github.com/kubernetes-client/javascript/pull/854/files

I'd prefer to do that if it's possible since I think there are additional clean-ups/fixes in that PR.

@jportner
Copy link
Author

Looks like we need to cherry-pick this PR into the main branch

https://github.com/kubernetes-client/javascript/pull/854/files

I'd prefer to do that if it's possible since I think there are additional clean-ups/fixes in that PR.

Thank you! I didn't see that PR before.

It appears all of the other changes from that PR (including tests) are already incorporated into the main branch, see #2129 (comment)

I also took a look just to make sure, and I came to the same conclusion. So I think my linked PR is probably all that needs to be changed to get main to parity unless you saw something else that I missed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants