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

docs(README.md): Re-write README.md for the Swagger-based client #222

Merged
merged 1 commit into from
Mar 15, 2018

Conversation

silasbw
Copy link
Contributor

@silasbw silasbw commented Mar 10, 2018

No description provided.

@silasbw silasbw force-pushed the readme0 branch 2 times, most recently from 734cd5b to ba23079 Compare March 10, 2018 05:36
//
// Remove the Deployment we created.
//
const removed = await await client.apis.apps.v1.namespaces('default').deployments(deploymentManifest.metadata.name).delete();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an accidental syntax error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😿

//
// Demonstrate some of the basics.
//
const Client = require('kubernetes-client').SyncClient
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: missing semicolon.

@@ -0,0 +1,41 @@
//
Copy link
Contributor

@gisenberg gisenberg Mar 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this file confusing because of the usage of SyncClient - based on the naming, I expected to receive a client that had only synchronous methods, but on further investigation, the constructor is synchronous and the methods hanging off of it are async.

I'm not sure if this is confusing to others or just me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not you, it's me :) addressed in #225

@silasbw silasbw force-pushed the readme0 branch 2 times, most recently from 4365812 to 97412c6 Compare March 13, 2018 16:19
README-PRE-5.md Outdated
console.log(JSON.stringify(err || result, null, 2));
}

core.namespaces('default').replicationcontrollers('http-rc').get(print);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This uses the default namespace, but the description of the example on line 32 says it is going to use the my-project namespace - is this intentional?

README-PRE-5.md Outdated
### **Experimental** support for promises and async/await

kubernetes-client has **experimental** support for promises. If you
omit callbacks an HTTP method function (*e.g.*, `.get`), it will
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: comma after "omit callbacks"

README-PRE-5.md Outdated
or with `async/await`:

```js
print(null, await core.namespaces('default').replicationcontrollers('http-rc').get());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This calls attention to print being a little weird - maybe just console.log the result?

README-PRE-5.md Outdated

```js
const Api = require('kubernetes-client');
const api = new Api.Api({ url: 'http://my-k8s-api-server.com' });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this code get easier to read if line 115 turns into `const Api = require('kubernetes-client').Api;``?

const thirdPartyResources = new Api.ThirdPartyResources({
url: 'http://my-k8s-api-server.com',
group: 'kubernetes-client.io',
resources: ['customresources'] // Notice pluralization!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: customresources => customResources

Copy link
Contributor Author

@silasbw silasbw Mar 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the lower case is correct for the (deprecated) third party resources.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:(

README-PRE-5.md Outdated

// Access `customresources` as if they were a regular Kubernetes object
thirdPartyResources.ns('default').customresources.get(print);
thirdPartyResources.addResource('newresources'); // Notice pluralization!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: newresources => newResources

README-PRE-5.md Outdated
});
```

**Note:** the kube-apiserver will close watch connections eventually
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this difficult to parse. Maybe something like this?

the kube-apiserver eventually closes watch connections

README-PRE-5.md Outdated
command line argument. kubernetes-client does not attempt to reconnect
when the kube-apiserver closes a connection.

### Authenticating
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Authenticating or Authentication?

@silasbw silasbw force-pushed the readme0 branch 2 times, most recently from 748081c to ae715ed Compare March 13, 2018 19:35
BREAKING CHANGE: This commit effectively pivots the Swagger-based client to be
the default, which isn't 100% compatible with the original kubernetes-client
implementation.
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.

2 participants