Promises based API #37

kjvalencik · 2016-10-28T12:30:04Z

It would be nice if this library also supported promises. Unfortunately, wrapping the request library for promises is fairly non-trivial. This makes it a bit unwieldy to promisify at runtime.

The request-promise library also supports a nodeify method for callbacks, allowing simulataneous support for callbacks and promises. However, this may make streaming more cumbersome.

Perhaps a provide your own request library approach?

The text was updated successfully, but these errors were encountered:

silasbw · 2016-11-08T23:36:35Z

Sorry for the delay @kjvalencik -- this is something we'd consider implementing, but it hasn't been high on our priority list because we usually eschew promises.

I think the first step would be rejiggering kubernetes-client a little bit to facilitate adding promise support. If you have some concrete thoughts here, we'd love to hear them.

Second step would then be actually adding the promise support. Either as an option to kubernetes-client, or as a separate kubernetes-client-promises wrapper package.

kjvalencik · 2016-11-09T15:00:57Z

There are a few things that I think could be helpful to allow integrating promises externally:

Ensure all calls return their result. This is also useful if you want to stream the response.
Optional config to pass the request library in.
There are a few places in the code that don't check that a parameter exists before grabbing a value off it. These work because a callback is usually passed. Without it you will get an undefined deref error.
Pass json: true to request so that parsing happens in the library instead of in a callback.

Open questions:

How to implement retry logic that works regardless of request library?
What to do with methods that do work in a wrapping callback before handing off to a user callback?
How to test to prevent regressions in promise functionality?

silasbw · 2016-11-09T17:25:14Z

@kjvalencik helpful list -- if we could rejigger things so that switching to promise mode was as easy as passing in the request library, that would be great. I'd worry about making the kubernetes-client code opaque and cumbersome to maintain, but that's a qualitative thing that would be hard to evaluate without looking at a PR first.

re: json: true -- there's reason we didn't do that to begin with and it's that when set and content-type: application/strategic-merge-patch+json, request (essentially) ignores json: true so our client code ends up implementing the JSON decoding anyways. To be consistent we just don't set it. We've meant to address this issue via a PR to request, but haven't done it yet.

…e to `{}` This fixes stream calls without an `options` argument. E.g.,: ``` // This used to TypeError. // Now it returns a stream. const stream = core.ns.po.get(); ``` #37 (comment)

silasbw · 2017-06-17T20:03:27Z

We should consider adding a promise-based API to kubernetes-client:

const api = require('kubernetes-client/async');
const core = new api.Core(api.config.fromKubeconfig());

async function main() {
  const pods = await core.ns.po.get();
  console.log(`Watching: ${ JSON.stringify(pods, null, 2) }`);
  const stream = core.ns.po.getStream({ qs: { watch: true }});
  stream.on('data', data => { console.log(data.toString()); });
  stream.on('error', err => { throw err; });
  stream.on('end', () => { console.log('end'); });
}
main();

Any feedback on the following:

including a promised-based API (v.s., an external wrapper kubernetes-client-promise);
[method]Stream functions (e.g., getStream);
API location and name (kubernetes-client/async);

or something else?

…e to `{}` This fixes stream calls without an `options` argument. E.g.,: ``` // This used to TypeError. // Now it returns a stream. const stream = core.ns.po.get(); ``` #37 (comment)

…e to `{}` (#115) This fixes stream calls without an `options` argument. E.g.,: ``` // This used to TypeError. // Now it returns a stream. const stream = core.ns.po.get(); ``` #37 (comment)

TWood67 · 2017-06-20T19:49:09Z

@silasbw

I think it would be best to include the promised base API in this module. I can see the advantages of having an external wrapper, but I've seen plenty of them neglected because they aren't a priority. Separation would also make regression tests for promises more difficult to manage, IMO.
I can think of one additional option here... We could return a Promise for calls to .stream. I like the solution here
Streams and promises petkaantonov/bluebird#332 (comment)
However this doesn't handle stream.on('data', dataHandler) so we would need to write something for that.
I would lean towards something like kubernetes-client/promise. async doesn't imply a promise library.

This change facilitates a Promised-based API (see [1] for discussion). The original `request`-like behavior is unchanged: return a stream if the caller omits a callback. [1]: #37

This preserves the original callback-based "core" implementation and layers the promised-based API on top of that core implementation. The goal is start using and iterating on the promised-based API even if we expect the underlying implementation to evolve (e.g., we could consider replacing the core callback-based implementation with a promise one). See #37 for discussion. Example usage: ```js async function main() { try { const pods = await core.ns('kube-system').po.get(); console.log(`Watching: ${ JSON.stringify(pods, null, 2) }`); const stream = core.ns('kube-system').po.getStream({ qs: { watch: true }}); stream.on('data', data => { console.log(data.toString()); }); stream.on('error', err => { throw err; }); stream.on('end', () => { console.log('end'); }); } catch (error) { console.log(error); } } main(); ```

This preserves the original callback-based "core" implementation and layers the promised-based API on top of that core implementation. The goal is start using and iterating on the promised-based API even if we expect the underlying implementation to evolve (e.g., we could consider replacing the core callback-based implementation with a promise one). See #37 for discussion. Example usage: ```js const core = new api.Core({ url: 'http://my-k8s-api-server.com', promises: true }); async function main() { try { const pods = await core.ns('kube-system').po.get(); console.log(`Watching: ${ JSON.stringify(pods, null, 2) }`); const stream = core.ns('kube-system').po.getStream({ qs: { watch: true }}); stream.on('data', data => { console.log(data.toString()); }); stream.on('error', err => { throw err; }); stream.on('end', () => { console.log('end'); }); } catch (error) { console.log(error); } } main(); ```

This change facilitates a Promised-based API (see [1] for discussion). The original `request`-like behavior is unchanged: return a stream if the caller omits a callback. [1]: #37

This preserves the original callback-based "core" implementation and layers the promised-based API on top of that core implementation. The goal is start using and iterating on the promised-based API even if we expect the underlying implementation to evolve (e.g., we could consider replacing the core callback-based implementation with a promise one). See #37 for discussion. Example usage: ```js const core = new api.Core({ url: 'http://my-k8s-api-server.com', promises: true }); async function main() { try { const pods = await core.ns('kube-system').po.get(); console.log(`Watching: ${ JSON.stringify(pods, null, 2) }`); const stream = core.ns('kube-system').po.getStream({ qs: { watch: true }}); stream.on('data', data => { console.log(data.toString()); }); stream.on('error', err => { throw err; }); stream.on('end', () => { console.log('end'); }); } catch (error) { console.log(error); } } main(); ```

silasbw · 2017-09-28T03:23:03Z

Added experimental support with 3.16.0.

…e to `{}` (#115) This fixes stream calls without an `options` argument. E.g.,: ``` // This used to TypeError. // Now it returns a stream. const stream = core.ns.po.get(); ``` godaddy/kubernetes-client#37 (comment)

This change facilitates a Promised-based API (see [1] for discussion). The original `request`-like behavior is unchanged: return a stream if the caller omits a callback. [1]: godaddy/kubernetes-client#37

This preserves the original callback-based "core" implementation and layers the promised-based API on top of that core implementation. The goal is start using and iterating on the promised-based API even if we expect the underlying implementation to evolve (e.g., we could consider replacing the core callback-based implementation with a promise one). See godaddy/kubernetes-client#37 for discussion. Example usage: ```js const core = new api.Core({ url: 'http://my-k8s-api-server.com', promises: true }); async function main() { try { const pods = await core.ns('kube-system').po.get(); console.log(`Watching: ${ JSON.stringify(pods, null, 2) }`); const stream = core.ns('kube-system').po.getStream({ qs: { watch: true }}); stream.on('data', data => { console.log(data.toString()); }); stream.on('error', err => { throw err; }); stream.on('end', () => { console.log('end'); }); } catch (error) { console.log(error); } } main(); ```

silasbw self-assigned this Jun 17, 2017

silasbw mentioned this issue Jun 17, 2017

fix(undefined options passed to .get): set default options value to {} #115

Merged

silasbw pushed a commit that referenced this issue Jun 23, 2017

feat(Add getStream to resource API)

0173c90

This change facilitates a Promised-based API (see [1] for discussion). The original `request`-like behavior is unchanged: return a stream if the caller omits a callback. [1]: #37

silasbw pushed a commit that referenced this issue Jun 23, 2017

feat(Add getStream to resource API)

bdcb2b8

This change facilitates a Promised-based API (see [1] for discussion). The original `request`-like behavior is unchanged: return a stream if the caller omits a callback. [1]: #37

silasbw mentioned this issue Jun 23, 2017

feat(Add getStream to resource API) #119

Merged

silasbw added a commit that referenced this issue Jul 6, 2017

feat(Add getStream to resource API) (#119)

6690327

This change facilitates a Promised-based API (see [1] for discussion). The original `request`-like behavior is unchanged: return a stream if the caller omits a callback. [1]: #37

silasbw mentioned this issue Jul 9, 2017

feat(Promise API): Experimental support for a promise-based API #126

Closed

silasbw mentioned this issue Jul 22, 2017

feat(Promise API): Experimental support for a promise-based API #133

Merged

silasbw closed this as completed Sep 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Promises based API #37

Promises based API #37

kjvalencik commented Oct 28, 2016

silasbw commented Nov 8, 2016

kjvalencik commented Nov 9, 2016

silasbw commented Nov 9, 2016

silasbw commented Jun 17, 2017

TWood67 commented Jun 20, 2017

silasbw commented Sep 28, 2017

Promises based API #37

Promises based API #37

Comments

kjvalencik commented Oct 28, 2016

silasbw commented Nov 8, 2016

kjvalencik commented Nov 9, 2016

silasbw commented Nov 9, 2016

silasbw commented Jun 17, 2017

TWood67 commented Jun 20, 2017

silasbw commented Sep 28, 2017