Move collection API endpoints from /rocketfuel to /feed (bug 958597) #1671

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

chuckharmston commented Jan 23, 2014

@chuckharmston chuckharmston commented on the diff Jan 23, 2014

docs/api/topics/feed.rst
@@ -394,3 +394,426 @@ Delete
:status 204: successfully deleted.
:status 403: not authorized.
+
+
+.. _feed-collections:
+
+-----------
@chuckharmston

chuckharmston Jan 23, 2014

Contributor

Changed header levels to match and update URLs, but this is basically copied from the rocketfuel docs.

Member

cvan commented Jan 24, 2014

what happens to rocketfuel and fireplace?

Contributor

chuckharmston commented Jan 24, 2014

@cvan This is only for API v2; endpoints remain working as normal for v1. Rocketfuel is being superseded by transonic, and when feed work is merged, Fireplace will be using the new endpoints.

Member

cvan commented Feb 4, 2014

I'll let @diox r+

Member

diox commented Feb 4, 2014

We discussed this on IRC and I wasn't thrilled by the idea of duplicating rocketfuel endpoints, I fear it might cause perf issues (our url routes are already bloated) and weird bugs (since the same urls are routed 3 times now). If possible I would prefer to continue using rocketfuel and do some version checks where necessary inside. If not well I can reconsider my position and r+ this :)

Contributor

chuckharmston commented Feb 20, 2014

My case for it is this:

  1. Rocketfuel will no longer exist. It's being superseded by transonic
  2. The purpose will fundamentally change, even though we're continuing to use the same models.
  3. They won't be solely rocketfuel-specific anymore, as other projects could use collections or feed endpoints for other purposes.
    • For example, today there was discussion about using Marketplace to host a collection of app templates that exist as apps in the Marketplace.
  4. Having an app-specific prefix sucks. We have the opportunity with v2 of the API to make things better. Why not do it?

Thoughts @diox?

@diox diox commented on the diff Feb 20, 2014

mkt/api/v2/urls.py
urlpatterns = patterns('',
+ url(r'^rocketfuel/collections/.*', endpoint_removed),
@diox

diox Feb 20, 2014

Member

I don't think you need the .*

@chuckharmston

chuckharmston Feb 26, 2014

Contributor

That's correct, removed in a forthcoming update.

Member

diox commented Feb 20, 2014

Fair enough. Edit: We should have a plan in place to remove /rocketfuel/ at some point though (since it's private, only rocketfuel and fireplace use this, it shouldn't be too hard)

I have one remaining concern: how do we unit test this ?

Contributor

chuckharmston commented Feb 20, 2014

This would be more of an integration test than a unit test, yeah?

The testing question in general is a good one, though. We would have to hardcode URLs, which is pretty bad. Open to suggestions, perhaps @robhudson has an idea?

Member

ngokevin commented Mar 10, 2014

I don't have any qualms about this since it's v2.

Contributor

chuckharmston commented Mar 17, 2014

Closing because #1869.

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