Skip to content
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

Versioned combinators and access_role changes in V3 #2841

Merged
merged 19 commits into from
Dec 2, 2022

Conversation

pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Nov 10, 2022

This PR introduces VersionedReqBody and VersionedRespond combinators for Servant and some V3 changes to the access_role field in conversation endpoints. The versioned combinators are used to select different instances for different versions, while keeping the handler types intact.

This supersedes #2741, which had diverged too much to be sensibly rebased or merged.

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@pcapriotti pcapriotti temporarily deployed to cachix November 10, 2022 10:05 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix November 10, 2022 10:05 Inactive
@pcapriotti pcapriotti changed the title New access role API Versioned combinators and access_role changes in V3 Nov 10, 2022
@pcapriotti pcapriotti force-pushed the pcapriotti/versioned-request-body branch from b5b6577 to d1d6068 Compare November 10, 2022 13:05
@pcapriotti pcapriotti temporarily deployed to cachix November 10, 2022 13:05 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix November 10, 2022 13:05 Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Nov 10, 2022
@pcapriotti pcapriotti temporarily deployed to cachix November 10, 2022 14:15 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix November 10, 2022 14:15 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix November 14, 2022 14:43 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix November 14, 2022 14:43 Inactive
@pcapriotti pcapriotti force-pushed the pcapriotti/versioned-request-body branch from bd078bb to 54dd410 Compare November 16, 2022 10:24
@pcapriotti pcapriotti temporarily deployed to cachix November 16, 2022 10:24 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix November 16, 2022 10:24 Inactive
@pcapriotti pcapriotti force-pushed the pcapriotti/versioned-request-body branch from 54dd410 to e16e7bb Compare November 17, 2022 08:36
@pcapriotti pcapriotti temporarily deployed to cachix November 17, 2022 08:36 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix November 17, 2022 08:36 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix November 17, 2022 09:10 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix November 17, 2022 09:10 Inactive
@pcapriotti pcapriotti force-pushed the pcapriotti/versioned-request-body branch from c42540f to 2a8b46d Compare November 21, 2022 09:42
@pcapriotti pcapriotti temporarily deployed to cachix November 21, 2022 09:42 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix November 21, 2022 09:42 Inactive
@pcapriotti pcapriotti force-pushed the pcapriotti/versioned-request-body branch from 2a8b46d to 5e60af8 Compare November 21, 2022 10:19
@pcapriotti pcapriotti temporarily deployed to cachix November 21, 2022 10:19 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix November 21, 2022 10:19 Inactive
@@ -282,3 +282,36 @@ Many variations on this theme are possible. For example, one could choose to
write adapting functions in terms of the new input/output types, or even use a
mixed approach. The adapting functions need not be pure in general, and they
might even perform further RPC calls.

## Versioning changes in events
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very abstract and could benefit from concrete examples of a timeline including multiple backend and client releases, and at which point changes occur. The way I currently read this is "The backend can make incompatible event changes as long as the client can deal with both the old format and the new format". That just moves the burden of backwards compatibility from the server to the clients, which is not ideal as old versions of clients are around for much longer than old versions of the backend. What am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the access_role change in this PR is an example of change X.

I added an example timeline. Not sure it adds anything useful, though, as it's mostly restating the same things with example numbers instead of N and Q.

That just moves the burden of backwards compatibility from the server to the clients, which is not ideal as old versions of clients are around for much longer than old versions of the backend. What am I missing?

The burden of backwards compatibility is shared with this system, and goes away after some time. At first both the backend and the clients have to support two formats. At some point in the future, one of the two can drop the old format. And finally the other one can drop it as well. In fact, the system is pretty symmetric wrt backend and clients.

I agree that it would be nicer to properly version events, but that seems to be quite a major undertaking, and might have some performance implications. This requires some extra bookkeeping, but ultimately achieves the same effect of getting rid of legacy code from all code bases, which is the ultimate goal anyway.

@pcapriotti pcapriotti temporarily deployed to cachix November 22, 2022 14:54 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix November 22, 2022 14:54 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix November 23, 2022 09:19 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix November 23, 2022 09:19 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix December 1, 2022 09:37 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix December 1, 2022 14:41 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix December 1, 2022 14:41 Inactive
@pcapriotti pcapriotti force-pushed the pcapriotti/versioned-request-body branch from 4a97097 to b6c87d3 Compare December 1, 2022 15:49
@pcapriotti pcapriotti temporarily deployed to cachix December 1, 2022 15:49 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix December 1, 2022 15:49 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix December 1, 2022 16:51 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix December 1, 2022 16:51 Inactive
@pcapriotti pcapriotti merged commit fb4a935 into develop Dec 2, 2022
@pcapriotti pcapriotti deleted the pcapriotti/versioned-request-body branch December 2, 2022 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants