fix: remove final refrences to request + fix tests #1033

rbnayax · 2023-03-31T08:07:53Z

I removed the request library (had to install form-data instead of it)
I added the node-fetch types
fixed log, watch, catch and metrics
Fixed all tests

linux-foundation-easycla · 2023-03-31T08:07:57Z

The committers listed above are authorized under a signed CLA.

✅ login: rbnayax (fb88d4c)

k8s-ci-robot · 2023-03-31T08:08:02Z

Welcome @rbnayax!

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. 😃

brendandburns · 2023-04-03T00:49:49Z

src/watch_test.ts


 describe('Watch', () => {
-    describe('DefaultRequest', () => {


I'm a little worried that we're deleting these two tests, are they really not needed?

those 2 are not relevant anymore as we don't use the DefaultRequest class anymore

brendandburns · 2023-04-03T00:50:25Z

src/watch_test.ts

        expect(doneErr.length).to.equal(1);
        expect(doneErr[0]).to.deep.equal(errIn);
-    });
-
-    it.skip('should handle server side close correctly', async () => {


Worried that we're removing this test too.

my bad, I readded but it is now skipped due to the node-fetch bug I mentioned (at least I think). To properly close a stream, one needs to call destroy which other tests are already performing and it works

Actually streams can't be closed anymore simply by emitting close events, to I think that test might be redundant

I fixed the test

brendandburns · 2023-04-03T00:50:43Z

src/watch_test.ts


-        expect(reqOpts.uri).to.equal(`${server}${path}`);


Are there equiavelent tests we could do for fetch?

The fact that the test passes, means the correct uri was used, as this is the URL nock will listen on. If you change the URL for some reason, you will get an error from nock

brendandburns · 2023-04-03T00:51:39Z

Generally this looks ok (and thanks for the PR!) but I'm a little bit worried that the test coverage is being reduced. Is there a way to maintain the test coverage?

rbnayax · 2023-04-03T08:02:53Z

Generally this looks ok (and thanks for the PR!) but I'm a little bit worried that the test coverage is being reduced. Is there a way to maintain the test coverage?

I did everything I can to test what is possible. If you can suggest other tests I will gladly implement

davidgamero · 2023-04-03T15:23:37Z

src/config.ts

@@ -146,6 +147,26 @@ export class KubeConfig implements SecurityAuthentication {
        this.makePathsAbsolute(rootDirectory);
    }

+    public async applytoFetchOptions(opts: https.RequestOptions): Promise<RequestInit> {


could we just merge this with applyToHTTPSOptions()?

We can not, as applyToHTTPSOptions is being used by proto-client

davidgamero · 2023-04-03T15:25:40Z

src/log.ts

-            done = doneOrOptions;
-        } else {
+    ): Promise<AbortController> {
+        const AbortControllerCtor = globalThis.AbortController || (await import('abort-controller'));


im not familiar with this pattern, why can't we just re-use the AbortController import from above?

AbortController exists natively since node since v15, so we can use it, or the "shim" in previous version

davidgamero · 2023-04-03T15:30:15Z

src/watch_test.ts

-            });
-        });
-
-        it('should handle non-200 error codes', (done) => {


i think we could convert this test to still validate this chunk of error wrapping by emitting a non-200 code and catching it on the stream?:

await fetch(watchURL, requestInit) .then((response) => { if (response.status === 200) { response.body.on('error', doneCallOnce); response.body.pipe(stream); } else { const error = new Error(response.statusText) as Error & { statusCode: number | undefined; }; error.statusCode = response.status; throw error; } }) .catch(doneCallOnce);

i see you have it in other tests, but this one was isolated to just that functionality

test should handle error from request stream does exactly that, unless I'm missing something here

rbnayax · 2023-04-09T09:49:23Z

src/config_test.ts

+            const expectedCA = Buffer.from('CADATA2', 'utf-8');
+            const expectedAgent = new https.Agent({
+                ca: expectedCA,
+                rejectUnauthorized: false,


this is a test... not sure how to ignore that

rbnayax · 2023-04-09T09:49:50Z

Generally this looks ok (and thanks for the PR!) but I'm a little bit worried that the test coverage is being reduced. Is there a way to maintain the test coverage?

I managed to find a way to maintain all existing tests so coverage is the same!

brendandburns · 2023-04-14T01:18:29Z

/lgtm
/approve

Thanks for the patience!

k8s-ci-robot · 2023-04-14T01:18:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, rbnayax

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

fix: remove unused request dependency

fb88d4c

k8s-ci-robot requested review from brendandburns and drubin March 31, 2023 08:07

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Mar 31, 2023

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 31, 2023

rbnayax mentioned this pull request Mar 31, 2023

Proposal to swap deprecated request for node-fetch #754

Closed

13 tasks

rbnayax marked this pull request as draft March 31, 2023 08:15

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 31, 2023

rbnayax added 2 commits March 31, 2023 16:54

fix: remove unused request dependency

69fa6b8

fix: remove unused request dependency

92ec46c

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 31, 2023

fix: remove unused request dependency

0ea7835

rbnayax marked this pull request as ready for review March 31, 2023 16:56

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 31, 2023

rbnayax changed the title ~~fix: remove unused request dependency~~ fix: remove final refrences to request + fix tests Mar 31, 2023

brendandburns reviewed Apr 3, 2023

View reviewed changes

rbnayax added 2 commits April 3, 2023 10:42

fix: remove unused request dependency

94561cc

fix: remove unused request dependency

465a920

davidgamero reviewed Apr 3, 2023

View reviewed changes

rudyflores mentioned this pull request Apr 4, 2023

Export credential manager API from CLI plugin zowe/zowe-cli-secrets-for-kubernetes#10

Merged

fix: remove unused request dependency

82c82bd

fix: remove unused request dependency

38ab1ca

github-advanced-security bot found potential problems Apr 5, 2023

View reviewed changes

rbnayax added 2 commits April 9, 2023 10:26

fix: remove unused request dependency

31e2ff9

fix: remove unused request dependency

6ae196d

commjoen mentioned this pull request Apr 12, 2023

Replace Request with something better maintained OWASP/wrongsecrets-ctf-party#212

Open

3 tasks

k8s-ci-robot assigned brendandburns Apr 14, 2023

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 14, 2023

brendandburns mentioned this pull request Apr 14, 2023

fix: casing of applyToHttpsOptions #1038

Merged

k8s-ci-robot merged commit 305c307 into kubernetes-client:release-1.x Apr 14, 2023

Nokel81 mentioned this pull request Apr 18, 2023

Attempt upgrade to @kubernetes/[email protected] lensapp/lens#7418

Closed

fix: remove final refrences to request + fix tests #1033

fix: remove final refrences to request + fix tests #1033

Uh oh!

Conversation

rbnayax commented Mar 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Mar 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 31, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendandburns commented Apr 3, 2023

Uh oh!

rbnayax commented Apr 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Check failure

Choose a reason for hiding this comment

Uh oh!

rbnayax commented Apr 9, 2023

Uh oh!

brendandburns commented Apr 14, 2023

Uh oh!

k8s-ci-robot commented Apr 14, 2023

Uh oh!

Uh oh!

rbnayax commented Mar 31, 2023 •

edited

Loading

linux-foundation-easycla bot commented Mar 31, 2023 •

edited

Loading