Skip to content

Comments

API versioning#2297

Merged
pcapriotti merged 35 commits intodevelopfrom
pcapriotti/api-versioning
May 9, 2022
Merged

API versioning#2297
pcapriotti merged 35 commits intodevelopfrom
pcapriotti/api-versioning

Conversation

@pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Apr 20, 2022

Implement API versioning system. See docs/legacy/developer/api-versioning.md for details.

Tracked by:

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • If this PR changes development workflow or dependencies, they have been A) automated and B) documented under docs/developer/. All efforts have been taken to minimize development setup breakage or slowdown for co-workers.
  • If HTTP endpoint paths have been added or renamed, the endpoint / config-flag checklist (see Wire-employee only backend wiki page) has been followed.
  • changelog.d contains the following bits of information (details):
    • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.
    • If new config options introduced: added usage description under docs/reference/config-options.md
    • If new config options introduced: recommended measures to be taken by on-premise instance operators.

@pcapriotti pcapriotti temporarily deployed to cachix April 20, 2022 11:44 Inactive
@pcapriotti pcapriotti force-pushed the pcapriotti/api-versioning branch from 36d0755 to 6b2faf8 Compare April 20, 2022 12:21
@pcapriotti pcapriotti temporarily deployed to cachix April 20, 2022 12:21 Inactive
@pcapriotti pcapriotti force-pushed the pcapriotti/api-versioning branch from 6b2faf8 to e1fee79 Compare April 20, 2022 13:17
@pcapriotti pcapriotti temporarily deployed to cachix April 20, 2022 13:17 Inactive
@pcapriotti pcapriotti force-pushed the pcapriotti/api-versioning branch from e1fee79 to ac1eaf3 Compare April 21, 2022 07:37
@pcapriotti pcapriotti temporarily deployed to cachix April 21, 2022 07:37 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix April 25, 2022 12:09 Inactive
@pcapriotti pcapriotti mentioned this pull request Apr 26, 2022
5 tasks
@pcapriotti pcapriotti force-pushed the pcapriotti/api-versioning branch from bdd2e8d to 1f9110f Compare April 26, 2022 09:55
@pcapriotti pcapriotti temporarily deployed to cachix April 26, 2022 09:55 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix April 26, 2022 11:15 Inactive
@pcapriotti pcapriotti force-pushed the pcapriotti/api-versioning branch from 659c5c7 to 2a68cdd Compare April 29, 2022 06:08
@pcapriotti pcapriotti temporarily deployed to cachix April 29, 2022 06:08 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix April 29, 2022 12:04 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix April 29, 2022 12:23 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix May 2, 2022 07:52 Inactive
@pcapriotti pcapriotti force-pushed the pcapriotti/api-versioning branch from f2c20da to 1058fbf Compare May 2, 2022 11:33
@pcapriotti pcapriotti temporarily deployed to cachix May 2, 2022 11:33 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix May 2, 2022 12:58 Inactive
@pcapriotti pcapriotti marked this pull request as ready for review May 2, 2022 12:58
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

I like the outcome of this process! I think it's both pragmatic and reasonably elegant. I have a few nit-picks, and haven't read enough of this PR to approve it, but I'm generally happy.

Start version 2 of the public API. Main changes:

- Asset endpoints have lost their `v3` and `v4` suffixes. So for example
`/assets/v3` has been replaced by `/access`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I hope this is a typo? :)

Suggested change
`/assets/v3` has been replaced by `/access`.
`/assets/v3` has been replaced by `/assets`.

Also, isn't that something that could have gone into a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could've, but I thought it would be useful to start using the new system immediately. It's kind of like adding tests for X in the same PR where you implement X itself. In fact, it is a bit worrying that we are not making significant use of it for the federation API. I've played around with it in a branch, though, by making some random changes to the API, and it seemed to be working reasonably well there, too.

# Versions and servant routing tables
Of course, development versions are useful while building a new API, but are
not suitable for production. Backends deployed to production environments
should disable development versions (and not advertise them in `/api-version`).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
should disable development versions (and not advertise them in `/api-version`).
should disable development versions (and not advertise them in `/api-version`).
On the other hand, production clients must disregard all development versions
as unsupported.

This would be easier if development versions would not also be listed as versions. I'm actually in favor of this. I think it's a lot less error-prone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went back and forth with this, and in the end I think I prefer the current system, because it places no burden on clients. Clients can just start using V2 now, without any special logic, and as long as we disable it in production, it won't affect anything.

Also, it makes it easier to test, because we only need to make sure that the backend has a development version enabled, and not that the client has turned out development API access as well.

But I can be convinced otherwise. This is not too problematic to change later on, anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added the following paragraph:

Similarly, clients that are meant for production use can decide to ignore
development versions on their backend. This is not strictly necessary, but it
can be used as a safeguard against mistakes in deployment.

Copy link
Contributor

Choose a reason for hiding this comment

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

I went back and forth with this, and in the end I think I prefer the current system, because it places no burden on clients. Clients can just start using V2 now, without any special logic, and as long as we disable it in production, it won't affect anything.

Also, it makes it easier to test, because we only need to make sure that the backend has a development version enabled, and not that the client has turned out development API access as well.

But I can be convinced otherwise. This is not too problematic to change later on, anyway.

I'm ok with everything except the last sentence. :) So, yes, do it your way.

@pcapriotti pcapriotti temporarily deployed to cachix May 3, 2022 05:27 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix May 3, 2022 05:27 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix May 3, 2022 05:30 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix May 3, 2022 05:30 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix May 3, 2022 05:37 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix May 3, 2022 05:37 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix May 3, 2022 05:41 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix May 3, 2022 05:41 Inactive
pcapriotti and others added 23 commits May 9, 2022 10:02
Since we are using file-embed-lzma to embed swagger definitions in the
executable (and servant-swagger itself is using it to embed the swagger
frontend), there is no point in maintaining a second way to embed static
files into the executable.

Therefore, this commit removes the mechanism for loading the swagger
description at compile time via a custom `Setup.hs`, introduced in
#1956, and simply embeds it
using `embedText` from `file-embed-lzma`.
Co-authored-by: fisx <mf@zerobuzz.net>
Co-authored-by: fisx <mf@zerobuzz.net>
Co-authored-by: fisx <mf@zerobuzz.net>
Also add an option to enable development versions.
@pcapriotti pcapriotti force-pushed the pcapriotti/api-versioning branch from 7f50245 to d2ee0f6 Compare May 9, 2022 08:04
@pcapriotti pcapriotti temporarily deployed to cachix May 9, 2022 08:05 Inactive
@smatting smatting self-requested a review May 9, 2022 09:12
@pcapriotti pcapriotti merged commit 7150c5c into develop May 9, 2022
@pcapriotti pcapriotti deleted the pcapriotti/api-versioning branch May 9, 2022 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants