The committers listed above are authorized under a signed CLA.

• ✅ login: jportner / name: Joe Portner (4c1e6c2, 0170e82, 6354cd7, 85007ed)

Welcome @jportner!

It looks like this is your first PR to kubernetes-client/javascript 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-client/javascript has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

Changes here LGTM. It looks like after these changes, main contains all of the changes from #854 except some minor differences which I believe are a result of the different underlying clients.

/lgtm

Hrm, our CI/CD doesn't appear to be triggering, probably because we need to update the github action to include the main branch.

@jportner sorry, but would you mind rebasing this to pick up #2132.

FYI @brendandburns this is what I see. I don't have permission to start the CI:

@jportner thanks for rebasing. Still LGTM

/lgtm

@cjihrig ok, I think you need to add yourself here:

https://github.com/kubernetes/org/blob/main/config/kubernetes-client/org.yaml#L141

Also, let's remove @davidgamero from admin since he hasn't been super involved lately (we can leave him as approver/reviewer)

/lgtm

Is this unit testable? If so, we should add unit tests.

/lgtm

Is this unit testable? If so, we should add unit tests.

Well I can't think of a way to test the looser type definition directly.
But I did make a second change to the existing unit test for this function in 4c1e6c2, which demonstrates that the problem is fixed by the first change.

If you temporarily revert the first change in 85007ed, you can see that there's now a type error in the tests. I tried to push a temporary revert to demonstrate that but it looks like the GitHub workflow wouldn't run again until I got another LGTM label 😅

Anyway, with the first change temporarily reverted (HEAD: 0170e82) if you try npm run build-with-tests you'll see the type error:

npm run build-with-tests

> @kubernetes/[email protected] build-with-tests
> tsc --project tsconfig-with-tests.json && cp 'src/test/echo space.js' dist/test

src/object_test.ts:1762:66 - error TS2345: Argument of type '{ apiVersion: string; kind: string; metadata: { name: string; namespace: string; }; }' is not assignable to parameter of type 'CustomTestResource'.
  Property 'spec' is missing in type '{ apiVersion: string; kind: string; metadata: { name: string; namespace: string; }; }' but required in type 'CustomTestResource'.

1762             const custom = await client.read<CustomTestResource>({
                                                                      ~
1763                 apiVersion: 'example.com/v1',
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 ... 
1768                 },
     ~~~~~~~~~~~~~~~~~~
1769             });
     ~~~~~~~~~~~~~

  src/object_test.ts:1728:17
    1728                 spec: {
                         ~~~~
    'spec' is declared here.


Found 1 error in src/object_test.ts:1762

Are you satisfied with that?

/lgtm

I tried to push a temporary revert to demonstrate that but it looks like the GitHub workflow wouldn't run again until I got another LGTM label

Someone from within the org needs to approve CI runs for outside collaborators.

@brendandburns is there any specific reason this is active for CI runs which do testing, linting, etc.?
I think there are ways to prevent workflow run changes from within a branch to be used in a workflow run (it will use the workflow definition from main if configured correctly).
Would be easier for outsiders to contribute and reduces the feedback cycle loop.

@mstruebing there are security concerns w/ running random PRs from unknown users automatically. People can do things like craft malicious PRs that mine Bitcoin, etc.

I think we want a maintainer to look at each PR before we approve it for running tests on our compute.

That test looks good enough to me, I mainly want something that will catch if we regress this change somehow. I think this fits the bill. Thanks!

/lgtm
/approve

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, jportner, saurabh2590

The full list of commands accepted by this bot can be found here.

The pull request process is described here