Skip to content

compat: Warn on unsupported or soon-unsupported server versions #5218

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

Merged
merged 2 commits into from
Feb 2, 2022

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Feb 2, 2022

This will let us get a better idea of how many (or few) users
will be affected as we drop support for an old version, or as
we remove compatibility hacks for old versions we already say
we don't support.


In particular, I have a draft branch to start relying on server 2.0+, following up on #5192. One of the main things it does is clean up how we send messages, to use stream IDs and user IDs instead of emails and stream names. That'll be a much-appreciated improvement… but it will also mean we break some pretty core functionality for any users interacting with a server that's still on a pre-2.0 version.

We announced rather a while ago, the middle of last year, that we were no longer supporting such old versions. We even started showing a warning banner for any users on such a server, and haven't heard a peep from users seeing it. So we aren't going to shy away from making that change. But it might be the cue for us to start outright refusing to operate on those servers, as discussed at 401b3a2. Knowing how many users are still in this situation will help us decide whether to build that now.

This will also help us decide when it's a good time to drop support for server 2.1 and start requiring server 3.x+, and so on in the future.

This makes it available for use outside the `logging` module.

Also take the opportunity to give it some test cases.
This will let us get a better idea of how many (or few) users
will be affected as we drop support for an old version, or as
we remove compatibility hacks for old versions we already say
we don't support.
Copy link
Contributor

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Glad to have this. Please merge at will.

*
* This should be the next major Zulip Server version after kMinSupportedVersion.
*/
export const kNextMinSupportedVersion: ZulipVersion = new ZulipVersion('3.0');
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the "k" for "constant"? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. This is influenced by Google style:
https://google.github.io/styleguide/cppguide.html#Constant_Names

I don't have any desire to go sweeping through existing code, but I'm dissatisfied with SCREAMING_SNAKE_CASE because it's more of a pain to type, and also more different from the other names we use. So wanted to experiment with this style.

@gnprice gnprice merged commit 78f80bc into zulip:main Feb 2, 2022
@gnprice
Copy link
Member Author

gnprice commented Feb 2, 2022

Merged. Thanks for the review!

@gnprice gnprice deleted the pr-compat-warn branch February 2, 2022 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants