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

Adding lots of resources & methods #52

Merged
merged 28 commits into from
Feb 1, 2018
Merged

Adding lots of resources & methods #52

merged 28 commits into from
Feb 1, 2018

Conversation

tavogel
Copy link
Contributor

@tavogel tavogel commented Jan 25, 2018

Hey there! We needed a good deal of resources & methods that weren't exposed by the library, and have added them. Hope this is useful!

Build-configs

  • Added findAll
  • Updated find - added missing buildconfigname check
  • Added update
  • Added removeAll

Builds

  • Added findAll
  • Updated find - added missing buildname check
  • Added create
  • Added update
  • Added removeAll

Cluster-role-bindings

  • Full new findAll, find, create, remove

ConfigMaps

  • Added findAll
  • Added create
  • Added update
  • Added remove
  • Added removeAll

DeploymentConfig

  • Added findAll
  • Updated find - added missing deploymentconfigname check
  • Added update
  • Added removeAll

Groups

  • Full new findAll, find, create, remove, removeAll

ImageStreams

  • Added findAll
  • Updated find - added missing imagestreamname check
  • Added update
  • Added removeAll

Pods

  • Full new findAll, find, create, remove, removeAll

ProjectRequests

  • Full new findAll, create

Projects

  • BREAKING CHANGE find() was used for findAll-type, behaviour so has been renamed (have updated README.md to reflect this). The find() method now expects a project name, like all other resources.
  • Added findAll
  • Added create
  • Added update
  • Added remove
  • Added removeAll

Replication Controllers

  • Added create
  • Added update

Role-bindings

  • Full new findAll, find, create, remove, removeAll

Routes

  • Added findAll
  • Updated find - added missing routename check
  • Added update
  • Added removeAll

Secrets

  • Added update

Services

  • Added findAll
  • Updated find - added missing servicename check
  • Added update
  • Added removeAll

- Resynced and added missing CRUD methods where possible
- More consistent options default declaration in function params
- Consistent promise rejection for missing name param
- Ordering, spacing, etc. all aligned. There's a lot of dupe/boilerplate, maybe this could be refactored into a higher order CRUD object that takes in a resource name and base URL...
- Slight style alignment with promise rejection message
@ghost ghost added the in progress label Jan 25, 2018
@coveralls
Copy link

coveralls commented Jan 25, 2018

Coverage Status

Coverage increased (+3.4%) to 97.011% when pulling 8e5d255 on telusdigital:master into f8f23c8 on bucharest-gold:master.

@lholmquist
Copy link
Member

Wow! :)

@lance
Copy link
Member

lance commented Jan 25, 2018

@tavogel I haven't had a chance to really review this yet, but just wanted to say thanks for such a comprehensive effort! Are you free to say what project you are working on with this module?

@tavogel
Copy link
Contributor Author

tavogel commented Jan 25, 2018

Hey @lance, the tool is our "Shippy" internal provisioning API/CLI/UI. We discuss it briefly during our talk from OpenShift Commons: https://youtu.be/GRjz775Na-M?t=1038

@lholmquist
Copy link
Member

@tavogel i'm starting to go through this and i noticed the comments, which i sort of use as documentation, are being removed. any reason for this?

@tavogel
Copy link
Contributor Author

tavogel commented Jan 26, 2018

@lholmquist Reason primarily being that I rebuilt all of the modules from a common template (copy paste, find replace). Every resource has the same basic functions... findAll, find, create, update, remove, removeAll (depending on the resource, not all are included). Some functions were documented, but most were not (and not all of the same type, for all resources). Some documentation was incorrect/misleading, or pointing at differing versions of the OpenShift documentation. If these comments are high value to you i'd recommend using JSDoc across all functions, and picking an OpenShift version to target.

Really the only things that change between the modules is the name of the resource, whether it uses Kube or OpenShift API, whether it is namespaced, and some resources don't have all of the methods defined. If you use your diff editor you can compare any two resource files together and you'll see those are the only things that are changed between these files. With a bit of copy/paste, find/replace, you can create a new resource in a minute or two. Same goes for the unit tests!

Since this pattern is so repeatable i'd probably recommend having a common, shared "resource" module, and just passing in a configuration object (you already have a higher order client wrapper -- it could go in there). This would define the name of the resource, the base url, which methods to include, and then return the same modules to you (without duplication). Then adding new resources is a matter of configuration. Added benefit: you wouldn't need as many duped tests either. I would have taken a stab at it but I thought it would be more challenging to merge, so I simply adopted your style for now.

Here's one example of this, from another OSS lib we're using in Shippy: https://github.com/kr1sp1n/node-vault/blob/master/src/commands.js

@lholmquist
Copy link
Member

@tavogel yea, the lib could be a little more simplified, but, thats a future thing :)

So the reason we only had certain methods added and not others was because we didn't need them at the time. This lib is used by another lib called nodeshift, https://github.com/bucharest-gold/nodeshift. So i only added the endpoints i needed.

what you've added is awesome!

@lholmquist
Copy link
Member

@tavogel i think i'm ok with all this stuff. @lance did you have any thoughts?

@lholmquist lholmquist merged commit 319b375 into nodeshift:master Feb 1, 2018
@ghost ghost removed the in progress label Feb 1, 2018
@lholmquist
Copy link
Member

@tavogel thanks for this. i'll release a new version soon

@lholmquist
Copy link
Member

@tavogel just released 0.11.0 https://www.npmjs.com/package/openshift-rest-client 🍾 🎆

@tavogel
Copy link
Contributor Author

tavogel commented Feb 1, 2018

Happy to help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants