Set v7 protocol as current version in docs #272
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… other implementations to this doc
…e was generated by an older version of the generator
amcohen-twitch
suggested changes
Sep 14, 2020
draft_release_notes.md
Outdated
| @@ -0,0 +1,15 @@ | |||
| A major release that includes a protocol spec update. The jump from v5 to v7 is to avoid confusion with issues and comments that were targeting v6 and were never released (see [achieved protocol spec v6](https://twitchtv.github.io/twirp/docs/spec_v6.html)). | |||
There was a problem hiding this comment.
achieved => archived?
draft_release_notes.md
Outdated
|
|
||
| Changes in the Protocol spec v7: | ||
|
|
||
| * Twirp routes can have any prefix: `<prefix>/<package>.<Service>/<Method>` section. In v5 the prefix was mandatory was always `/twirp`. |
There was a problem hiding this comment.
was mandatory was always => was mandatory, always
draft_release_notes.md
Outdated
|
|
||
| * #264 Optional Twirp Prefix. Implements the proposal #263 with the optional prefix, using an option to specify a different prefix than the default value "/twirp". The default value ensures backwards compatibility when updating the service. Using different prefixes, it is now possible to mount the same Twirp service in multiple routes, which may help migrating existing services from the "/twirp" prefix to a new one. | ||
| * #264 also introduces [server options on the server constructor](https://github.com/twitchtv/twirp/pull/264#issuecomment-686170407). Adding server hooks should now done through a server option. The server options are available in the new version of the `twirp` package, servers generated with the new version require the `twirp` package to be updated as well. | ||
| * #270 ResourceExhausted error code to HTTP 429 status code. To implement the new spec for this error, so it is clear that this error code can be used for rate limiting. This change may affect middleware if it depends on a specific status code being used. |
There was a problem hiding this comment.
To implement the new spec for this error, so it is clear that this error code can be used for rate limiting.
This sentence doesn't quite make sense.
There was a problem hiding this comment.
😄 what was I thinking? Updated. Thanks!
thesilentg
reviewed
Sep 15, 2020
PROTOCOL.md
Outdated
| | already_exists | 409 | An attempt to create an entity failed because one already exists. | ||
| | permission_denied | 403 | The caller does not have permission to execute the specified operation. It must not be used if the caller cannot be identified (use "unauthenticated" instead). | ||
| | unauthenticated | 401 | The request does not have valid authentication credentials for the operation. | ||
| | resource_exhausted | 403 | Some resource has been exhausted, perhaps a per-user quota, or perhaps the entire file system is out of space. |
There was a problem hiding this comment.
Does this need to be updated to 429?
There was a problem hiding this comment.
Yes, this entire file should be a copy-paste form the spec in the docs. Which I though I did already 🤔 good catch. I wonder if this should just contain a link to the docs ... but realistically it almost never changes, so it's fine to have some duplication 🤷
There was a problem hiding this comment.
oh.. I see. I need to merge #270 first
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Set the v7 protocol as the current version. Include differences with v5 so we can link other implementations to this doc. This should be merged along with a new release that enumerates all the changes that were included in this release:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.