-
Notifications
You must be signed in to change notification settings - Fork 26
feat: initialise buf congiurations & generate yamls #250
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
Conversation
Signed-off-by: kushthedude <kushthedude@gmail.com>
|
Introspected Roadmap:-
|
|
Please ignore the generated-failure right now, I didn't regenerate the stubs as we will be deprecating protoc and move out to buf build |
shravanshetty1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kushthedude you also need to update the documentation in the blobbercore/blobbergrpc folder
| additional_bindings: { | ||
| delete: "/v2/file/upload/{allocation}" | ||
| body: "*" | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a delete api that accepts data in its body, you should handle this in a separate issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not possible, DELETE request body is explicitly discarded by the HTTP protocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI. where it is written, please read this :
The spec does not explicitly forbid or discourage it, so I would tend to say it is allowed.
Microsoft sees it the same way (I can hear murmuring in the audience), they state in the MSDN article about the DELETE Method of ADO.NET Data Services Framework:
If a DELETE request includes an entity body, the body is ignored [...]
Additionally here is what RFC2616 (HTTP 1.1) has to say in regard to requests:
an entity-body is only present when a message-body is present (section 7.2)
the presence of a message-body is signaled by the inclusion of a Content-Length or Transfer-Encoding header (section 4.3)
a message-body must not be included when the specification of the request method does not allow sending an entity-body (section 4.3)
an entity-body is explicitly forbidden in TRACE requests only, all other request types are unrestricted (section 9, and 9.8 specifically)
For responses, this has been defined:
whether a message-body is included depends on both request method and response status (section 4.3)
a message-body is explicitly forbidden in responses to HEAD requests (section 9, and 9.4 specifically)
a message-body is explicitly forbidden in 1xx (informational), 204 (no content), and 304 (not modified) responses (section 4.3)
all other responses include a message-body, though it may be of zero length (section 4.3)
However, not going into much details this issue should be separately handled.
Signed-off-by: kushthedude <kushthedude@gmail.com>
This will be done when the integration with buf is complete, there is a lot of semantic as well as definition error in the protos including the package versioning, best practices & generated stubs. Once it is done & buf is integrated into our CI pipelines then I think it would be better to remove the protoc with buf in the repository. |
Signed-off-by: kushthedude kushthedude@gmail.com
reference #43