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

Make use of types for websocket library #26

Closed

Conversation

cjlarose
Copy link
Contributor

This fixes a compilation error I was getting when trying to import this library. The generated exec.d.ts file looked like this:

/// <reference types="node" />
import stream = require('stream');
import { WebSocketHandler } from './web-socket-handler';
import { KubeConfig } from './config';
export declare class Exec {
    'handler': WebSocketHandler;
    constructor(config: KubeConfig);
    exec(namespace: string, podName: string, containerName: string, command: string, stdout: stream.Writable | any, stderr: stream.Writable | any, stdin: stream.Readable | any, tty: boolean): Promise<WebS
ocket>;
}

which produced this error

../../kubernetes-js-client/node-client/dist/exec.d.ts(8,201): error TS2304: Cannot find name 'WebSocket'.

I suspect that these changes also fix an error related to HTTP Basic Auth when using websockets connections because the auth property on the WebSocket connection was being set to an object instead of a ${username}:${password} string as NodeJS's HTTPS client expects.

@brendandburns
Copy link
Contributor

@cjlarose thanks for the PR, I just merged #39 so this needs a rebase, but then I think we can merge this...

@cjlarose
Copy link
Contributor Author

@brendanburns I think that #39 made a bunch of the same changes as this PR.

One notable difference is that #39 removed the use of this.config.applyToRequest when making a secure WebSockets connection, so some TLS options appear to no longer be passed (ca, cert, key). I don't know if those options are important or not. If they are, I think it's a regression, but regardless, it's a separate issue from this original PR.

@cjlarose cjlarose closed this Apr 25, 2018
@sheldonkwok
Copy link
Contributor

Yeah, I just noticed that as well. I created a new pull request here: #41

Rebasing yours works for me as well.

@cjlarose
Copy link
Contributor Author

@sheldonkwok I like yours better. Let's go with #41.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants