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

Implement strict RESTful API #214

Open
imeoer opened this Issue Sep 25, 2017 · 22 comments

Comments

Projects
None yet
6 participants
@imeoer

imeoer commented Sep 25, 2017

Expected Behaviour

The API definition of gateway and provider should be more in line with the RESTful semantic specification.

Note: Do not change existing endpoints/routes - so we can maintain compatibility

Current Behaviour

// List of deployed functions.
GET /system/functions
-> 200 Ok

// Deploy a new function.
POST /system/functions
-> 200 Ok

// Remove a deployed function.
DELETE /system/functions
-> 200 Ok
-> 404 Not Found

// Invoke a deployed function.
POST /function/$name
-> 200 Ok
-> 404 Not Found
-> 500 Internal Server Error

// Invoke a deployed function asynchronously
POST /async-function/$name
-> 202 Accepted
-> 404 Not Found
-> 500 Internal Server Error

// Event-sink for AlertManager
POST /system/alert
-> 200 Ok
-> 500 Internal Server Error

Possible Solution

// List of deployed functions.
GET /system/functions
-> 200 Ok
-> 500 Internal Server Error

// Deploy a new function.
POST /system/functions
-> 201 Created
-> 400 Bad Request
-> 500 Internal Server Error
-> 503 Service Unavailable (FaaS Provider Unreachable)

// Remove a deployed function.
DELETE /system/functions/$name
-> 204 No Content
-> 400 Bad Request
-> 404 Not Found
-> 500 Internal Server Error

// Update a deployed function.
PUT /system/functions/$name
-> 200 Ok
-> 400 Bad Request
-> 404 Not Found
-> 500 Internal Server Error

// Invoke a deployed function.
ANY /function/$name
-> 400 Bad Request
-> 200 Ok
-> 404 Not Found
-> 500 Internal Server Error

// Invoke a deployed function asynchronously
ANY /async-function/$name
-> 400 Bad Request
-> 202 Accepted
-> 404 Not Found
-> 500 Internal Server Error

// Event-sink for AlertManager
POST /system/alert
-> 400 Bad Request
-> 200 Ok
-> 500 Internal Server Error

Would be updating gateway, provider, related test cases and Swagger definition.

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Sep 25, 2017

Member

Can you do a current behaviour vs expected for this please? We also need to cover UPDATE function

Member

alexellis commented Sep 25, 2017

Can you do a current behaviour vs expected for this please? We also need to cover UPDATE function

@imeoer

This comment has been minimized.

Show comment
Hide comment
@imeoer

imeoer Sep 25, 2017

@alexellis Updated. where is the UPDATE definition in the Swagger doc?

imeoer commented Sep 25, 2017

@alexellis Updated. where is the UPDATE definition in the Swagger doc?

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Sep 25, 2017

Member

It is currently in progress in #208 @johnmccabe can you also update the Swagger when you have time?

Member

alexellis commented Sep 25, 2017

It is currently in progress in #208 @johnmccabe can you also update the Swagger when you have time?

@imeoer

This comment has been minimized.

Show comment
Hide comment
@imeoer

imeoer Sep 25, 2017

@alexellis I will try to update once have time.

imeoer commented Sep 25, 2017

@alexellis I will try to update once have time.

@johnmccabe

This comment has been minimized.

Show comment
Hide comment
@johnmccabe

johnmccabe Sep 25, 2017

Member

Derek add label: proposal

Member

johnmccabe commented Sep 25, 2017

Derek add label: proposal

@open-derek open-derek bot added the proposal label Sep 25, 2017

@alexellis alexellis changed the title from Better RESTful API Implementation to Implement strict RESTful API Sep 26, 2017

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Sep 26, 2017

Member

How can a GET form a bad request in this instance?

// List of deployed functions.
GET /system/functions
-> 400 Bad Request
-> 200 Ok

Member

alexellis commented Sep 26, 2017

How can a GET form a bad request in this instance?

// List of deployed functions.
GET /system/functions
-> 400 Bad Request
-> 200 Ok

@imeoer

This comment has been minimized.

Show comment
Hide comment
@imeoer

imeoer Sep 28, 2017

@alexellis Right, updated. :)

imeoer commented Sep 28, 2017

@alexellis Right, updated. :)

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Sep 28, 2017

Member

Derek add label: hacktoberfest

Member

alexellis commented Sep 28, 2017

Derek add label: hacktoberfest

@open-derek open-derek bot added the hacktoberfest label Sep 28, 2017

@VitorFalcao

This comment has been minimized.

Show comment
Hide comment
@VitorFalcao

VitorFalcao Oct 9, 2017

@alexellis I would like to start contributing to the project and this seems to be a nice start. May I have it? And do you have any further instructions? Thank you.

VitorFalcao commented Oct 9, 2017

@alexellis I would like to start contributing to the project and this seems to be a nice start. May I have it? And do you have any further instructions? Thank you.

@nullseed

This comment has been minimized.

Show comment
Hide comment
@nullseed

nullseed Oct 10, 2017

Contributor

@VitorFalcao have you started work on this?

Contributor

nullseed commented Oct 10, 2017

@VitorFalcao have you started work on this?

@VitorFalcao

This comment has been minimized.

Show comment
Hide comment
@VitorFalcao

VitorFalcao Oct 10, 2017

@nullseed I am currently familiarizing myself with the codebase and then I will implement it. If this is urgent and you want to solve it, then go ahead. If not, I would like to keep working on it, I find this project very interesting and would like to start contributing to it.

VitorFalcao commented Oct 10, 2017

@nullseed I am currently familiarizing myself with the codebase and then I will implement it. If this is urgent and you want to solve it, then go ahead. If not, I would like to keep working on it, I find this project very interesting and would like to start contributing to it.

@nullseed

This comment has been minimized.

Show comment
Hide comment
@nullseed

nullseed Oct 11, 2017

Contributor

@VitorFalcao I'll leave it to you then, feel free to run ideas by me here, I'm also interested in this issue.

Contributor

nullseed commented Oct 11, 2017

@VitorFalcao I'll leave it to you then, feel free to run ideas by me here, I'm also interested in this issue.

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Oct 11, 2017

Member

@nullseed this can be implemented on the faas-netes side too and faas-provider. Do you want to take that piece?

Member

alexellis commented Oct 11, 2017

@nullseed this can be implemented on the faas-netes side too and faas-provider. Do you want to take that piece?

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Oct 11, 2017

Member

Note: Do not change existing endpoints/routes - so we can maintain compatibility

Member

alexellis commented Oct 11, 2017

Note: Do not change existing endpoints/routes - so we can maintain compatibility

@Lewiscowles1986 Lewiscowles1986 referenced this issue Oct 16, 2017

Merged

Added optional body to function calls #303

8 of 12 tasks complete
@VitorFalcao

This comment has been minimized.

Show comment
Hide comment
@VitorFalcao

VitorFalcao Oct 17, 2017

@alexellis I would like to confirm what I would need to do. Basically: check every single route handler and set the w.WriteHeader(http.StatusOK) to the appropriate status, correct?

VitorFalcao commented Oct 17, 2017

@alexellis I would like to confirm what I would need to do. Basically: check every single route handler and set the w.WriteHeader(http.StatusOK) to the appropriate status, correct?

@nullseed

This comment has been minimized.

Show comment
Hide comment
@nullseed

nullseed Oct 26, 2017

Contributor

@VitorFalcao there's also identifying error paths and returning the appropriate error codes when an error occurs. It would be great to add some tests around these endpoints changes too. If you like i will take a stab at this one and you can look at my PR and use it as a reference for contributing to faas-netes and faas-provider. @alexellis where would you like the tests adding, in this repo or certify-incubator?

Contributor

nullseed commented Oct 26, 2017

@VitorFalcao there's also identifying error paths and returning the appropriate error codes when an error occurs. It would be great to add some tests around these endpoints changes too. If you like i will take a stab at this one and you can look at my PR and use it as a reference for contributing to faas-netes and faas-provider. @alexellis where would you like the tests adding, in this repo or certify-incubator?

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Oct 26, 2017

Member

This is work @imeoer committed to do. @imeoer are you still wanting to do this or can someone else pick it up?

@VitorFalcao basically I think the idea was to include the function name as part of the URL. You could start there with a PR to the openfaas/faas-provider repo (do not modify the existing routes).

Member

alexellis commented Oct 26, 2017

This is work @imeoer committed to do. @imeoer are you still wanting to do this or can someone else pick it up?

@VitorFalcao basically I think the idea was to include the function name as part of the URL. You could start there with a PR to the openfaas/faas-provider repo (do not modify the existing routes).

@nullseed nullseed referenced this issue Oct 26, 2017

Merged

[WIP] Return 500 if GET /system/functions fails #346

7 of 11 tasks complete
@imeoer

This comment has been minimized.

Show comment
Hide comment
@imeoer

imeoer Oct 27, 2017

@alexellis Sorry, I do not have time to do this now, hope someone else picks it up.

imeoer commented Oct 27, 2017

@alexellis Sorry, I do not have time to do this now, hope someone else picks it up.

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Oct 27, 2017

Member

Was it Harry Zhang that took over from you? Do you think he can pick it up?

Member

alexellis commented Oct 27, 2017

Was it Harry Zhang that took over from you? Do you think he can pick it up?

@nullseed

This comment has been minimized.

Show comment
Hide comment
@nullseed

nullseed Oct 27, 2017

Contributor

I'd like to continue work on this if no one else has started.

Contributor

nullseed commented Oct 27, 2017

I'd like to continue work on this if no one else has started.

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Dec 20, 2017

Member

@nullseed how far did you get with this? Do you still need more time to complete your changes?

cc/ @stefanprodan we spoke about this yesterday.

Member

alexellis commented Dec 20, 2017

@nullseed how far did you get with this? Do you still need more time to complete your changes?

cc/ @stefanprodan we spoke about this yesterday.

@stefanprodan

This comment has been minimized.

Show comment
Hide comment
@stefanprodan

stefanprodan Dec 20, 2017

Member

I propose to start versioning the API. This way we can ensure backwards compatibility with the CLI.
In the past I've used go-chi middleware, it has a nice way of handling API versioning, an example can be found here https://github.com/go-chi/chi/blob/master/_examples/versions/main.go

Member

stefanprodan commented Dec 20, 2017

I propose to start versioning the API. This way we can ensure backwards compatibility with the CLI.
In the past I've used go-chi middleware, it has a nice way of handling API versioning, an example can be found here https://github.com/go-chi/chi/blob/master/_examples/versions/main.go

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