@gempesaw feat(pod-exec): add initial support for command execution #329
Conversation
|
Cool! Maybe @3rd-Eden can give some advice on using client certs with |
gempesaw
changed the title
|
Cool, it turns out I had been experimenting without
I've added a few questions inline! Cheers. |
| 'resize' | ||
| ]; | ||
|
|
||
| /** |
There was a problem hiding this comment.
I'm not in the habit of writing JSDoc, so I'm ready to take any feedback here.
lib/request.js
Outdated
| * @param {function} cb - callback to handle the response | ||
| * @returns {string} All necessary query parameters as a string ready for inclusion in URL | ||
| */ | ||
| function buildExecQueryParams(qs, cb) { |
There was a problem hiding this comment.
can you use qs to implement this function? e.g.,
qs.stringify(qs, { indicies: false });There was a problem hiding this comment.
Oh, yes - that should make this much cleaner.
lib/request.js
Outdated
| const commandAsParams = command.map((cmd) => `command=${encodeURIComponent(cmd)}`); | ||
| delete qs.command; | ||
|
|
||
| const execParams = Object.assign(qs, { |
There was a problem hiding this comment.
Instead of adding these on implicitly, i'd just assume the caller specified them.
There was a problem hiding this comment.
Hmm, so the caller must know to include them in the body on every call? aka usage example would change to
client.api.v1.namespaces('namespace_name').pods('pod_name').exec.post({
qs: {
command: [ 'ls', '-al' ],
stdin: true, // hmm perhaps stdin isn't necessary, i'll have to double check
stdout: true,
stderr: true
}
})Unfortunately, the error response while I was forgetting to include stdout/stderr query params was just a 400 Error with no message, so it may be a bit difficult for users to debug... but if it's in the examples/ usage, hopefully that's enough guidance?
Happy to make the change, and agree it makes sense since kubernetes-client isn't in the habit of adding query parameters anywhere else, just wondering about troubleshooting :P
There was a problem hiding this comment.
Yeah, I'm suggesting that the user needs to include those on every call for the reason you mention about eschewing adding query parameters. I think the example/ you wrote will help. I think is there's enough interest and/or common patterns it could make sense to have a separate module focused on helping exec things (e.g., kubernetes-exec).
|
Ok, I think we're all set, based on the last round of feedback... |
lib/request.js
Outdated
|
|
||
| function upgradeRequest(options, cb) { | ||
| const queryParams = qs.stringify(options.qs, { indices: false }); | ||
| const wsUrl = `${options.baseUrl}/${options.uri}?${queryParams}`; |
gempesaw
changed the title
|
Heh heh, oops, good catch. Also, I rebased into one commit to try to match y'all's style. Cheers! |
This doesn't work at all yet - I'm just opening the PR early for fun. I'm currently stuck trying to figure out how to pass the ca, cert, & key for the WS connection. Whenever I finish, I'm planning to rebase to fit y'all's commit message format.edit: Huzzah, this is in a working state now; more details below.
fixes #19, fixes #319