-
Notifications
You must be signed in to change notification settings - Fork 192
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
feat(dynamic clients): introduce more user-friendly Client classes #207
Conversation
if (res.statusCode !== 200) { | ||
return reject(new Error(`Failed to get swagger.json: ${ res.statusCode }`)); | ||
} | ||
resolve(super({ http, spec: res.body })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to cause problems for Node < 6. Should we drop support for Node 4 and Node 5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Node 4 EOLs at the end of April : https://github.com/nodejs/Release , seems fine to not go out of our way at this point? It is slightly sad that we won't hit the EOL date, but don't see much sense in going out of our way for this.
examples/sync-client-version.js
Outdated
@@ -0,0 +1,20 @@ | |||
// | |||
// Create an API client based on a specified API version. kubernetes-client uses | |||
// included swagger specification files to implement this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we remove the "to implement this" bit, confused me on first read.
@@ -1,16 +1,24 @@ | |||
// | |||
// Deprecated interface | |||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have any documentation on this being deprecated? might want to add that too. Node generally does documentation -> runtime warning -> removal over time for their deprecation policy. IDK if we want to follow, but docs seem good either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Sounds good. I'm going to rewrite README.md next, so I'll start your suggested deprecation process with that re-write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have nit, but lgtm
BREAKING CHANGE: EOL for 4 is end of April (https://github.com/nodejs/Release). We're dropping support a little early in order to leverage some 6+ features for upcoming improvements (e.g., #207)
BREAKING CHANGE: EOL for 4 is end of April (https://github.com/nodejs/Release). We're dropping support a little early in order to leverage some 6+ features for upcoming improvements (e.g., #207)
Client and SyncClient abstract away the HTTP request adapter.
BREAKING CHANGE: EOL for 4 is end of April (https://github.com/nodejs/Release). We're dropping support a little early in order to leverage some 6+ features for upcoming improvements (e.g., godaddy/kubernetes-client#207)
Client and SyncClient abstract away the HTTP request adapter.