-
Notifications
You must be signed in to change notification settings - Fork 28
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
Refactor with new api #113
Conversation
3b357a4
to
c582668
Compare
Pull Request Test Coverage Report for Build 342
💛 - Coveralls |
@lance @helio-frota @danbev I know there is a lot of changes, but the majority of the changes are just deleting the unneeded files. All the important changes will be in https://github.com/nodeshift/openshift-rest-client/pull/113/files#diff-c0782fd6b5b7064b86ae2e67194217a5 and of course the README :) |
you can also try this out with |
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.
So much code deleted! It feels so good. 👍
@@ -40,7 +40,7 @@ | |||
"bugs": "https://github.com/nodeshift/openshift-rest-client/issues", | |||
"license": "Apache-2.0", | |||
"dependencies": { | |||
"openshift-config-loader": "^0.4.0", | |||
"kubernetes-client": "~6.11.0", |
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 you think openshift-config-loader
is going the way of the dinosaur?
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.
it might be. The kubernetes-client basically does the same thing, and it is a little better with the no SSL stuff too.
@@ -18,4 +18,7 @@ | |||
* | |||
*/ | |||
|
|||
module.exports = exports = require('./lib/openshift-rest-client.js'); | |||
module.exports = exports = { | |||
OpenshiftClient: require('./lib/openshift-rest-client.js'), |
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.
If you change the export in ./lib/openshift-rest-client.js
to be OpenshiftClient
you could just eliminate the key on this line (minor nit, really)
} | ||
|
||
// unzip and load the openshift openapi swagger spec file | ||
// Doesn't look like there is a way to query this from a running cluster | ||
// Perhaps, we could try to query https://raw.githubusercontent.com/openshift/origin/master/api/swagger-spec/openshift-openapi-spec.json ? |
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.
If you do decide to take this route, I think you'd want to not query master
but instead target a specific version.
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.
right, if we did do this, it would be in another PR/version i think
How did you enable this? |
|
BREAKING CHANGE: API is now different * The API is now generated based on the Openshift Open api spec * The api is fluent, for example, client.apis['build.openshift.io'].v1.namespace('default').builds.get() * Not a drop in replacement for version 1.x
connects to #71
This will be a Semver-major change.
It updates the library to use the kubernetes-client under the hood, and will now generate the whole openshift API automatically.
The api has changed, it is more "fluent"