CCI-MOC / hil

Hardware Isolation Layer, formerly Hardware as a Service
Apache License 2.0
24 stars 54 forks source link

Discuss: API should be versioned #983

Closed xuhang57 closed 6 years ago

xuhang57 commented 6 years ago

See details in: https://github.com/CCI-MOC/hil/issues/571

Basically, we should discuss whether we should get started for API versioning through an approach this patch purposes.

And to test:

  1. pull this PR
  2. install hil and configure hil and start hil server
  3. use:
    $ hil list_nodes free
    or
    $ curl -H "Content-Type: application/json" -X GET $HIL_ENDPOINT/nodes/free

    And for testing request methods that are not GET: for example,

    $ hil project_create test-project
    or
    $ curl -X put http://127.0.0.1:5000/project/mockproject01 -u hil:hil
xuhang57 commented 6 years ago

thanks Naved for sending this to me: https://stackoverflow.com/questions/389169/best-practices-for-api-versioning

I believe it worths us all to take a look 👍 @naved001 @Izhmash @zenhack

zenhack commented 6 years ago

So, I'm increasingly skeptical of the notion that REST is even a good idea for our use cases, and most of the arguments around "good REST design" seem 90% aesthetic, which for a tool that's supposed to be very low-level feels rather bikeshed. Early on I spent a lot of personal energy fussing about the way we do URIs and requests bodies etc. so that it felt "natural" in that pardigm, but the paradigm basically puts for thinking about URIs as resources, documents, and the use of the API as shunting data around, which is an awkward, clumsy way to model what HIL actually does. In retrospect, I think with something more RPC-ish we would have wasted a lot less time bikesheding the mapping of our design to HTTP.

There are important bits of thinking about an HTTP mapping; http verbs have specific semantics around things like caching, side effects, etc. But I've grown weary of arguments that basically amount to "it fits the metaphor better." Metaphors are there to help us understand something, not as a prescription.

From a very pragmatic perspective, I think putting the URL in the version is a good idea, becuase it forces the user to actually specify the version they're depending on. The arguments against it in that stack overflow thread are, frankly, coming more from a dogmatic view about "what REST should be" rather than any kind of serious thought about the problem. The REST metaphor makes sense in some cases, but I'm alergic to paradigms, especially as a way to guide pragmatic design decisions. Historically "good object oriented design" has been used to justify a lot of really crappy techincal decisions; see https://plus.google.com/+RobPikeTheHuman/posts/hoJdanihKwb for a good description. I worry REST has become the same thing to an extent.

We basically cargo-culted openstack when we started this project, which is universally a terribly idea that I've been pushing back on since very early on. Sometimes they do stuff that's sensible, but it's hit or miss, which is why I get a bit annoyed any time "openstack does this" comes up as an argument for something.

I guess what I'm getting at is I'd urge people to think about the concrete. This isn't a call to necessarily ignore aesthetics; a good developer experience is something that I care deeply about. From the closed tag on that post:

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise.

Sorry for the rant. I guess I feel like the a lot of the details of how to do this are pretty bikeshed, and I'm a bit weary of the discussion.

I also feel like a detailed design is premature; we haven't even talked about when we're going to start stabalizing the API, or what sort of guarantees we want to have. All I really want to do right now is tack '/v0' on to the front of our URLs so that existing tools are very clearly marked as pulling from the unstable API.

xuhang57 commented 6 years ago

We don't have a detailed plan of what to do with the API versioning so it is not bikeshed yet. Thanks @zenhack so much for the input.

Maybe we should spend some time discussing and writing some specs first so we can actually implement this. And I agree it is still premature for having a detailed plan for now.

In other words, now I do think putting a /v0 on to the front is a good idea. I will update this PR accordingly.

And as a side note, I don't know whether we should even go back to the RPC world. REST solves the problems and it just works. GraghQL might be the future and I honestly don't know how it will go. Admittedly HIL is obviously not like Facebook that has tons of nodes/trees/graph. But GraphQL does allow you to query the exact data you are looking for in one request and I believe it is very powerful.

Again, putting /v0 should give us what we need for now.

zenhack commented 6 years ago

Ok, so sounds like we're agreed on the immediate term: just add /v0 to the front of the URLs.

Re: rpc, I think our current API design is perfectly functional and it doesn't seem worth the effort to port everything over to something else. My only complaint is that there's a bit of a disconnect between how people tend to talk/think about it (basically procedure names) and the notion of "REST," and with each change to the API there's been some overhead in discussing how to map things to HTTP, which in retrospect I don't think really provided any value. An rpc design would have obviated those discussions. But again, I don't think it's a big enough deal to change course.

Re: GraphQL/other speculation: none of these approaches are universally applicable. They each have their use cases, but none of them are "the future" in any general sense. A lot of the ideas behind (all of) these designs have waxed and waned in mindshare over the years, resurging in new forms. Best to avoid the fads and think about actual requirements.

zenhack commented 6 years ago

This looks good to me; once the obmd tests are fixed assuming Travis passes I'm happy.

I'm working on debugging the Go stuff now.

naved001 commented 6 years ago

Couple of things:

xuhang57 commented 6 years ago

I rebased it. But these commits are still listed separately.

naved001 commented 6 years ago

hmmm, there's a bunch of test failures.

zenhack commented 6 years ago

Ah, yeah, looks like the /v0 needs to be added in a bunch of places in the tests as well.

xuhang57 commented 6 years ago

I am thinking of how we can create a global /v0 in the tests so that we only need to update one place. Maybe we should even put the version in one settings file?

zenhack commented 6 years ago

@xuhang57, I think there aren't actually that many places that need fixing:

Let's see how many are left once we fix those.

xuhang57 commented 6 years ago

I added the /v0 to a few places in the tests. However, it seems we are still having some problems with user operation testing. Still trying to figure out why.

zenhack commented 6 years ago

@xuhang57, looks like the client library is inserting an extra slash, which is causing the server to return a 404, since it can't match the path /v0//projects.

naved001 commented 6 years ago

the commit history is really weird :/ You should probably rebase this and pick the commits you want, or idk maybe squash it. I think rebasing this on top of master might fix it.

xuhang57 commented 6 years ago

okay, it is because in the client/project.py and client/switch.py, both of the list function have the extra / in the self.object_url.

xuhang57 commented 6 years ago

I am going to send a new PR. I messed up with the commit history