REST API irregularity #320

Closed
akavlie opened this Issue Feb 28, 2012 · 13 comments

Comments

Projects
None yet
2 participants

akavlie commented Feb 28, 2012

The Nodester API exposes the list of apps at GET /apps, while an individual app is at GET /app/<appname>. It seems that the root in both cases should be consistent (i.e. change the latter to GET /apps/<appname>.

Also, the API to start an app is as follows:

PUT /app?appname=<appname>&running=true

Conforming with the standard pattern, it should be like this:

PUT /apps/<appname>?running=true

I'm not just being pedantic... Backbone.js expects URLs like this.

akavlie commented Feb 28, 2012

Sorry... think this belongs in the nodester-api repo.

Collaborator

chrismatthieu commented Feb 28, 2012

Hi Aaron,

I agree with your assessment of the API. These recommendations will make it better. We just need to coordinate the updates between the nodester platform, nodester-api, and possible the admin-panel. We also need to assign this work to someone.

Thanks,
Chris

akavlie commented Feb 28, 2012

You have to make changes in Nodester actual for the nodester-api to work like this?

Collaborator

chrismatthieu commented Feb 28, 2012

Nice!

akavlie commented Feb 28, 2012

Wow, awesome @mintplant
If the API is in this codebase, what does nodester-api do?

Also, are you concerned about breakage for other apps that might be using the current API?

@ghost

ghost commented Feb 28, 2012

Oh, hey, that shows up here. I didn't get a chance to update nodester-api or admin-panel yet. nodester-api is a client module for the API, used by the cli, for example.

RE: breakage, perhaps the old routes could be left in as a fallback?

EDIT: Updated the API - https://github.com/mintplant/nodester-api/tree/issue-320

akavlie commented Feb 28, 2012

Don't worry about admin-panel @mintplant, unless you really want to. I'm in the middle of rewriting the whole front end.

@ghost

ghost commented Feb 29, 2012

Oh, nice! Shiny new admin panel.

Okay, well, I added back in the old routes, and marked them as deprecated. API docs updated too. Going to send these in as pull requests.

akavlie commented Feb 29, 2012

Yeah, well the "shiny new" will mostly be in the code. Help would be welcome if you have any design chops.

@ghost

ghost commented Feb 29, 2012

Sure, I can lend a hand, if needed.

akavlie commented Feb 29, 2012

Would love to have some design help... the current design is pretty rough, but my ability to improve it is limited.

btw, best to take the conversation off of this message thread... I can't message you though. You need to add your email address to your github profile.

@ghost

ghost commented Feb 29, 2012

Ah, yes, heh. It's there now.

nodester added a commit that referenced this issue Mar 1, 2012

Merge pull request #324 from mintplant/master
Changed API in accordance with #320.

alejandro added a commit that referenced this issue Mar 4, 2012

Merge branch 'master' of https://github.com/nodester/nodester
* 'master' of https://github.com/nodester/nodester: (24 commits)
  updated FAQ about node.js versions
  added alejandromg to nodester about page - core team member
  updated apps post docs
  changed access method of appname in put and post
  changed access method of appname in put and post
  changed access method of appname in put and post
  changed access method of appname in put and post
  changed access method of appname in put and post
  added appname to path on apps put and post
  modified the permissions on create directory to see if it fixes 296 and or 313
  updated cli page - added new env commands
  This prevents one from doing, say, nodester npm install -g my_malicious_package
  Updated API docs. (#320)
  Added back in old routes, marking them as deprecated. (#320)
  added mobi to list of valid TLD
  'okay no ternary :<'
  "add radix to parseInt"
  "switch to ternary operator"
  'made errorPage a local variable'
  Changed API according to #320.
  ...
Collaborator

chrismatthieu commented Mar 13, 2012

API updates have been deployed to Nodester and Nodester-API and Nodester-CLI. Thanks @akavlie for your great recommendation and thanks @mintplant for your excellent code updates!

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