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

not possible to set a client timeout #1613

Open
cdbkr opened this issue Mar 12, 2024 · 7 comments
Open

not possible to set a client timeout #1613

cdbkr opened this issue Mar 12, 2024 · 7 comments
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. javascript Pull requests that update Javascript code

Comments

@cdbkr
Copy link

cdbkr commented Mar 12, 2024

Describe the bug
At the moment, it seems not possible to set a client timeout since node-fetch requires passing an AbortController instance to the fetch request option's signal.

** Client Version **
1.0.0-rc3

To Reproduce

Expected behavior
It would be great to set a client's timeout when creating a k8sApi instance or invoking a request

@mstruebing mstruebing added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. javascript Pull requests that update Javascript code labels Mar 12, 2024
@elizabeth-dev
Copy link

/assign

@elizabeth-dev
Copy link

hmm, I'm having a bit of trouble with this issue, not because of the implementation, but because how the library's API is structured

right now, the configuration object (where undoubtedly we would need to store the config value for the timeout) is built in this function, but we'd need to add a new parameter to makeApiClient to store that additional config parameter (and right now nothing more, although I'm considering the possibility of also adding a custom http.Agent option in a different PR), and although it wouldn't be breaking, it feels like a big enough change to try and get the opinion of some more experienced contributors.

tl;dr: in order to implement this feature, we'd need to change the signature of the makeApiClient method to accept an additional parameter, a config object. would this be acceptable?

@mstruebing
Copy link
Member

I would say yes but it should definitely be an optional argument and probably an object so that it's easier to extend and adapt in the future.

@brendanburns any concerns regarding this?

@brendandburns
Copy link
Contributor

brendandburns commented Apr 22, 2024

I think it is totally fine to add an optional typed parameter to the makeApiClient signature. Thanks for checking!

@elizabeth-dev
Copy link

After starting to work on this and looking a bit more into it, I've come to the conclusion that this can not be implemented right now 😕. All the code that handles the HTTP requests is generated with the library openapi-generator-maven-plugin, and the client is completely coupled to that. I thought there would be some way to customize the template that generates the request (at least add some extra parameters for the timeout), but it doesn't look like it.

So 90% of the functionality for this feature needs to be done on the OpenAPI generator (and I'm assuming it'd need to be done across all the languages they support).

@elizabeth-dev elizabeth-dev removed their assignment Apr 24, 2024
@mstruebing
Copy link
Member

@elizabeth-dev I did not look to deep into it, but I think this needs to be implemented in: https://github.com/OpenAPITools/openapi-generator/tree/master/modules/openapi-generator/src/main/resources/typescript

(or typescript-fetch/typescript-node, where you would be able to only change the code for a specific case and do not need to implement it for all. I may be wrong here though.

@brendandburns
Copy link
Contributor

I think that this is the place where you would want to add the timeout code:

https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/typescript-fetch/runtime.mustache#L81

In general that project is pretty good about merging PRs for specific runtimes.

As I splunked through the code though, I realized you can also make the change here:

https://github.com/kubernetes-client/javascript/blob/release-1.x/src/config.ts#L470

And I'm pretty sure that it will work. That's the Configuration object that flows down through to fetch ultimately (if I tracked the code right)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. javascript Pull requests that update Javascript code
Projects
None yet
Development

No branches or pull requests

4 participants