bump openapi version to support for k8s media types #1341

davidgamero · 2023-10-13T16:03:14Z

regenerate after bumping the open api version to include OpenAPITools/openapi-generator#16386

ref #754
this should allow the routes that are currently blocked by missing media types, and i've included an example for patch showing how it can be done

the next priority would ideally be a fully custom object serializer, or simply adding the ability to merge header maps in the options parameter.

For now, this will provide a way to get things working even if it's a bit verbose

brendandburns · 2023-10-16T17:37:43Z

/lgtm
/approve

davidgamero · 2023-10-16T20:50:17Z

this is weird we need more than 2gb of memory to run our tests now?

mstruebing · 2023-10-17T08:05:59Z

examples/typescript/patch/patch-example.ts

+                requestContext.setHeaderParam('Content-type', 'application/json-patch+json');
+                return requestContext;
+            },
+            post: async (responseContext: ResponseContext) => responseContext,


Hey, one question:
is it required to define both pre as well as post even if you do not do anything else than return the arguments like in your example with post?

i can try removing them right now and let you know, but I had them to satisfy the typescript typings and show what was possible, not necessarily because they are needed

i don't see any default implementations of the pre/post methods, so they are most likely required for now, but we could easily add a wrapper that does default them or even a wrapper that makes middleware/configuration from a desired MIME media type since that's most of what we're after here

update: you can skip the middleware altogether if you want to use the first supported content type which is application/json-patch+json, but for any other types you'll have to add it, but this is just a workaround for now, and the ergonomics will be greatly improved with config merging in the future :)

Thank you for explaining. I was just asking because I was curious :)
This is fine, especially as a workaround! 👍

davidgamero · 2023-10-27T18:02:13Z

i was able to locally repo this using the ubuntu 22.04 docker image and running the container with a 2gb memory limit, but when limiting node memory with node --max_old_space_size=25 $(which npm) test i was able to run the tests on as little as 25mb.

I'm continuing to test so we can get his merged

davidgamero · 2023-10-27T18:08:59Z

testing this out for now actions/runner-images#70 (comment)

See also kubernetes-client/javascript#1341

This had many different effects that derived from it, making this PR so massive. **Modules**: The first stage was to rewrite all the imports to use .js endings everywhere. Then we needed to replace and bump a bunch of dependencies to their ESM version. **Bundling**: Following that we tried to use rollup to bundle everything back into CommonJS so that we can use it in pkg since pkg does not support ESM. This was unsuccessful, and after trying several different single application solutions, we decided we will have to write our own. **Single Executable Binary**: We now use a custom Rust binary which bundles a zipped version of NodeJS, the native extensions and the bundled code. We bundle the code for tree-shaking and size reduction, but also because otherwise we had issues with how we ensure that native modules are loaded correctly and with resolving the imports correctly cross-platform. Then on first run, the binary extracts those files to the filesystem and runs node from there. There are checksum files that we bundle together with the archives and that we store on disk to see if we need to re-extract or not. This also gives us a huge performance boost in some cases - especially when lots of filesystem reads are required - since pkg was patching and proxying those methods and making things slow. **Kubernetes client**: We also had to update the Kubernetes library to the 1.0.0-rc3, with a patch from a fork that contains the changes for kubernetes-client/javascript#1341 That is because the library was using request-promise behind the scenes, which started showing up with unhandled rejection warnings and errors about req not being defined in some cases. That library update required us to change almost every k8s call since the interface now uses objects instead of positional arguments, which makes things much nicer to read and use. We also had to introduce a workaround for using custom certificates together with proxies, something that was previously globally monkey-patched by global-agent. **request-promise**: We no longer depend on request-promise and fully removed the dependency. Fixes #3841 Fixes #4898

This had many different effects that derived from it, making this PR so massive. **Modules**: The first stage was to rewrite all the imports to use .js endings everywhere. Then we needed to replace and bump a bunch of dependencies to their ESM version. **Bundling**: Following that we tried to use rollup to bundle everything back into CommonJS so that we can use it in pkg since pkg does not support ESM. This was unsuccessful, and after trying several different single application solutions, we decided we will have to write our own. **Single Executable Binary**: We now use a custom Rust binary which bundles a zipped version of NodeJS, the native extensions and the bundled code. We bundle the code for tree-shaking and size reduction, but also because otherwise we had issues with how we ensure that native modules are loaded correctly and with resolving the imports correctly cross-platform. Then on first run, the binary extracts those files to the filesystem and runs node from there. There are checksum files that we bundle together with the archives and that we store on disk to see if we need to re-extract or not. This also gives us a huge performance boost in some cases - especially when lots of filesystem reads are required - since pkg was patching and proxying those methods and making things slow. **Kubernetes client**: We also had to update the Kubernetes library to the 1.0.0-rc3, with a patch from a fork that contains the changes for kubernetes-client/javascript#1341 That is because the library was using request-promise behind the scenes, which started showing up with unhandled rejection warnings and errors about req not being defined in some cases. That library update required us to change almost every k8s call since the interface now uses objects instead of positional arguments, which makes things much nicer to read and use. We also had to introduce a workaround for using custom certificates together with proxies, something that was previously globally monkey-patched by global-agent. **request-promise**: We no longer depend on request-promise and fully removed the dependency. Fixes #3841 Fixes #4898 Co-authored-by: Steffen Neubauer <steffen@garden.io>

mstruebing · 2023-11-07T10:22:39Z

this is weird we need more than 2gb of memory to run our tests now?

@davidgamero I needed to increase the memory limit in this PR as well in order to run the tests to even 8GB: #1388
That's kind of mad especially since it's running locally without such a huge memory.
I did not do anything else than executing npm audit fix

davidgamero · 2023-11-08T21:43:00Z

@mstruebing i just merged your changes from main and it looks like it works now, but i suspect one of the packages just had an issue that's somehow consuming extra CI memory

mstruebing

Hey @davidgamero some small comments but in general I'm happy with this PR :)

examples/typescript/patch/patch-example.ts

settings

brendandburns · 2023-11-14T01:38:19Z

@mstruebing I'll let you merge this when ready.

davidgamero · 2023-11-14T05:51:20Z

i was able to make some progress isolating the gc memory issue- maybe we make a separate issue to track it

by executing nyc mocha with the --node-command inspect to enable debugging i was able to connect a memory profiler and get a GC snapshot and allocation timeline

the majority of the growing heap usage seems to be coming from SourceMapConsumer in node_modules/source-map/lib/source-map-consumer.js at the bottom of a pretty long call stack that includes @ babel/core

i found this potentially relevant link that has encouraging similarities.

by changing nyc.instrument=false in package.json on line 51, the memory never exceeded 600MB, while re-enabling that line eventually balloons up to 12GB which was the most i was willing to test.

i will continue investigating upgrading our source-map indirect dependency so we can hopefully get this resolved

mstruebing · 2023-11-14T11:04:26Z

/lgtm
/approve

Thank you very much for this effort 🙏
I agree we should track and fix the memory limit separately.

k8s-ci-robot · 2023-11-14T11:04:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, davidgamero, mstruebing, OliverMKing

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [brendandburns]

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

davidgamero added 2 commits October 13, 2023 09:48

regen with bumped openapi

d4862a4

update patch examples

f1e230b

k8s-ci-robot requested review from brendandburns and drubin October 13, 2023 16:03

k8s-ci-robot added 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. labels Oct 13, 2023

OliverMKing approved these changes Oct 16, 2023

View reviewed changes

k8s-ci-robot assigned brendandburns Oct 16, 2023

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 16, 2023

davidgamero added 3 commits October 16, 2023 14:03

audit fix

5c059cc

audit fix

f3af15b

audit fix

c17ff31

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 16, 2023

mstruebing reviewed Oct 17, 2023

View reviewed changes

stefreak added a commit to garden-io/garden that referenced this pull request Nov 2, 2023

fix: due to bugs in kubernetes clients, we need to use a fork

b5f05be

See also kubernetes-client/javascript#1341

stefreak added a commit to garden-io/garden that referenced this pull request Nov 2, 2023

fix: due to bugs in kubernetes clients, we need to use a fork

e55b9e7

See also kubernetes-client/javascript#1341

TimBeyer mentioned this pull request Nov 2, 2023

chore: switch to using JavaScript Modules (ESM) garden-io/garden#5233

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2023

Merge branch 'release-1.x' into davidgamero/bump-openapi-for-media-types

7ce7322

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2023

mstruebing mentioned this pull request Nov 9, 2023

1.0.0-rc3 Error "None of the given media types are supported" #1398

Closed

mstruebing reviewed Nov 9, 2023

View reviewed changes

examples/typescript/patch/patch-example.ts Outdated Show resolved Hide resolved

examples/typescript/patch/patch-example.ts Outdated Show resolved Hide resolved

settings Outdated Show resolved Hide resolved

comment feedback

62e5ab1

k8s-ci-robot assigned mstruebing Nov 14, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2023

k8s-ci-robot merged commit 3987e4a into kubernetes-client:release-1.x Nov 14, 2023
7 checks passed

mstruebing mentioned this pull request Jan 17, 2024

Request is fully deprecated as of February 11 2020. #414

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bump openapi version to support for k8s media types #1341

bump openapi version to support for k8s media types #1341

davidgamero commented Oct 13, 2023

brendandburns commented Oct 16, 2023

davidgamero commented Oct 16, 2023

mstruebing Oct 17, 2023 •

edited

Loading

davidgamero Oct 17, 2023

davidgamero Oct 17, 2023

davidgamero Oct 17, 2023 •

edited

Loading

mstruebing Oct 17, 2023

davidgamero commented Oct 27, 2023

davidgamero commented Oct 27, 2023

mstruebing commented Nov 7, 2023

davidgamero commented Nov 8, 2023

mstruebing left a comment •

edited

Loading

brendandburns commented Nov 14, 2023

davidgamero commented Nov 14, 2023 •

edited

Loading

mstruebing commented Nov 14, 2023

k8s-ci-robot commented Nov 14, 2023

bump openapi version to support for k8s media types #1341

bump openapi version to support for k8s media types #1341

Conversation

davidgamero commented Oct 13, 2023

brendandburns commented Oct 16, 2023

davidgamero commented Oct 16, 2023

mstruebing Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

davidgamero Oct 17, 2023

Choose a reason for hiding this comment

davidgamero Oct 17, 2023

Choose a reason for hiding this comment

davidgamero Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

mstruebing Oct 17, 2023

Choose a reason for hiding this comment

davidgamero commented Oct 27, 2023

davidgamero commented Oct 27, 2023

mstruebing commented Nov 7, 2023

davidgamero commented Nov 8, 2023

mstruebing left a comment • edited Loading

Choose a reason for hiding this comment

brendandburns commented Nov 14, 2023

davidgamero commented Nov 14, 2023 • edited Loading

mstruebing commented Nov 14, 2023

k8s-ci-robot commented Nov 14, 2023

mstruebing Oct 17, 2023 •

edited

Loading

davidgamero Oct 17, 2023 •

edited

Loading

mstruebing left a comment •

edited

Loading

davidgamero commented Nov 14, 2023 •

edited

Loading