Skip to content

chore: regenerate#2770

Open
mstruebing wants to merge 26 commits intomainfrom
max-regen-gen
Open

chore: regenerate#2770
mstruebing wants to merge 26 commits intomainfrom
max-regen-gen

Conversation

@mstruebing
Copy link
Member

@mstruebing mstruebing commented Feb 14, 2026

This regenerates the with the new openapi generator after the changes from #2738

The code needs to be adjusted the generator now works wit undici.
Help would be appreciated, I've tried it two times but always went into deep rabbit holes.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mstruebing

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 14, 2026
@davidgamero
Copy link
Contributor

i started on this too, im working on the nock testing bit as undici fetch uses mocks instead

socks proxy was a rabbithole too i havent found a solution for that yet

@Rezrazi
Copy link

Rezrazi commented Feb 18, 2026

@mstruebing @davidgamero I tried to give it a go, however many apis use "fetch" directly, instead of using the same "configuration" pattern in other regular APIs.

Would you prefer if the APIs are to be rewritten to match the same pattern? in the spirit of my previous PR updating watch_api?

I think if all APIs are aligned, it would be easier to update to undici, but it will most definitely be a breaking change

@Rezrazi
Copy link

Rezrazi commented Feb 18, 2026

these apis currently use fetch() directly

  • src/health.ts
  • src/log.ts
  • src/metrics.ts
  • src/watch.ts

@brendandburns
Copy link
Contributor

@copilot please fix this PR

Copy link
Contributor

Copilot AI commented Mar 12, 2026

@brendandburns I've opened a new pull request, #2794, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI and others added 4 commits March 12, 2026 20:49
Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com>
Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com>
…gent

Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com>
fix: migrate API client HTTP transport from node-fetch to undici
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 17, 2026
@brendandburns
Copy link
Contributor

I tried a copilot fix for this which is now applied, it modifies a bunch of the tests, I did some manual verification, but would love a second pair of eyes.

@brendandburns
Copy link
Contributor

@copilot please fix tests, looks like File is not defined? Maybe this is a nodejs 18 thing?

Copy link
Contributor

Copilot AI commented Mar 17, 2026

@brendandburns I've opened a new pull request, #2797, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI and others added 2 commits March 17, 2026 23:43
Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com>
@brendandburns
Copy link
Contributor

@copilot please fix the remaining 3 failing tests.

Copy link
Contributor

Copilot AI commented Mar 17, 2026

@brendandburns I've opened a new pull request, #2798, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI and others added 2 commits March 17, 2026 23:52
Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com>
…nother-one

fix: correct incorrectly nested tests in object_test.ts
@brendandburns
Copy link
Contributor

@mstruebing @davidgamero All tests now appear to be passing.

If you two could take a look and make sure that we didn't regress our test coverage in these changes that'd be great.

I did my own scan and it seems to be ok, but it never hurts to have multiple eyes on the subject.

@copilot please do one last scan to make sure that our unit test coverage hasn't regressed because of this PR, if it has regressed, please add additional coverage to bring it back up to standard.

Copy link
Contributor

Copilot AI commented Mar 17, 2026

@brendandburns I've opened a new pull request, #2799, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI and others added 2 commits March 18, 2026 00:06
Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com>
…test data

Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com>
strictEqual(dispatcher instanceof UndiciAgent, true);
strictEqual(
dispatcher[
Object.getOwnPropertySymbols(dispatcher).find((s) => s.toString() === 'Symbol(options)')!
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking nit: this approach is fragile and is basically reaching into undici internals. I don't have a better suggestion though.

Comment on lines +138 to +139
// When skipTLSVerify is not set, no custom dispatcher is needed - undici validates certs by default
strictEqual(requestContext.getDispatcher(), undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem all that helpful for validating anything.

src/config.ts Outdated
if (cluster && cluster.proxyUrl) {
if (cluster.proxyUrl.startsWith('socks')) {
throw new Error(
'SOCKS proxy is not supported with the undici HTTP client. ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean we are dropping support for this feature?

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like this was just merged in the last 2weeks, nodejs/undici#4385
on undici 7.23.0 so there's a path to bringing it back without additional packages

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated undici and removed the throw and added a test - with the fragile approach for now.

strictEqual(dispatcher instanceof UndiciAgent, true);
strictEqual(
dispatcher[
Object.getOwnPropertySymbols(dispatcher).find((s) => s.toString() === 'Symbol(options)')!
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about this being fragile. I see it happens a few more times so I'll stop pointing it out.

Comment on lines +53 to +54
await mockAgent.close();
setGlobalDispatcher(originalDispatcher);
Copy link
Contributor

Choose a reason for hiding this comment

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

These should probably be in a t.after() hook around line 39.

throw new ApiException<undefined | V1Status>(
500,
`Error occurred in metrics request: ${e.message}`,
`Error occurred in metrics request: ${e.message}${e.cause ? ': ' + e.cause.message : ''}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if it makes sense in this case, but it seems like it would be more idiomatic to attach e as the cause of this error being thrown.

@@ -70,6 +70,7 @@
"socks-proxy-agent": "^8.0.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

Are any deps now unused? I was thinking along the lines of node-fetch, socks-proxy-agent, or nock? I didn't check to see if they are still used anywhere.

src/top_test.ts Outdated
Comment on lines +214 to +217
} finally {
await mockAgent.close();
setGlobalDispatcher(originalDispatcher);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment on all of these finally blocks about using a t.after() hook.

Copilot AI and others added 2 commits March 18, 2026 14:51
Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com>
…et-again

fix: restore unit test coverage for proxy agent paths in config.ts
@brendandburns
Copy link
Contributor

@copilot can you address the comments wrt to the finally block?

@cjihrig I think that socks proxy support is still there, since there is now a unit test for the SoxyProxyAgent showing up but if we get to the point of creating an undici dispatcher, which is HTTP, and the proxy is SOCKS, it will error out. This should really never happen. (at least that's my reading of the code)

Copy link
Contributor

Copilot AI commented Mar 18, 2026

@brendandburns I've opened a new pull request, #2800, to work on those changes. Once the pull request is ready, I'll request review from you.

Co-authored-by: brendandburns <5751682+brendandburns@users.noreply.github.com>
strictEqual(
(dispatcher as any)[
Object.getOwnPropertySymbols(dispatcher!).find((s) => s.toString() === 'Symbol(options)')!
].connect.servername,
Copy link
Contributor

Choose a reason for hiding this comment

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

i echo the previous comments on private state extraction of symbols @cjihrig
as many of these tests are essentially validating args to the UndiciAgent | UndiciProxyAgent constructors,

proposal to extract a createDispatcherOptions method from createDispatcher that shares a parameter signature ex:

private createDispatcherOptions(
  cluster: Cluster | null,
  agentOptions: https.AgentOptions
): UndiciAgentOptions | UndiciProxyAgentOptions 

we can test that method directly, and the createDispatcher function serves to pass those options to constructors exclusively.
this would remove the need for much of the fragile introspection

@davidgamero
Copy link
Contributor

@copilot try out the idea to extract a createDispatcherOptions method in a PR and note if it can effectively reduce reliance on symbol internals by testing the options generation instead of the objects post-constructor invocation:

proposal to extract a createDispatcherOptions method from createDispatcher that shares a parameter signature ex:

private createDispatcherOptions(
  cluster: Cluster | null,
  agentOptions: https.AgentOptions
): UndiciAgentOptions | UndiciProxyAgentOptions 

we can test that method directly, and the createDispatcher function serves to pass those options to >constructors exclusively.

@davidgamero
Copy link
Contributor

davidgamero commented Mar 19, 2026

made a PR into this branch for the options extraction here #2801

@brendandburns
Copy link
Contributor

@copilot please fix tests again, looks like the undici version got moved to version 7 again and it should be version 6

Copy link
Contributor

Copilot AI commented Mar 19, 2026

@brendandburns I've opened a new pull request, #2802, to work on those changes. Once the pull request is ready, I'll request review from you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants