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.

Sign up for GitHub

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

Jump to bottom

matrix-synapse: 1.3.1 -> 1.4.0 #70469

Merged
merged 1 commit into from Oct 14, 2019
Merged

matrix-synapse: 1.3.1 -> 1.4.0 #70469

merged 1 commit into from Oct 14, 2019

Conversation

Vskilet
Copy link
Contributor

@Vskilet Vskilet commented Oct 5, 2019

Motivation for this change

Latest release
https://github.com/matrix-org/synapse/releases/tag/v1.4.0

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @Ma27 @pacien

@ofborg ofborg bot requested review from Ekleog, pacien and Ralith October 5, 2019 15:07
@Ma27
Copy link
Member

Ma27 commented Oct 5, 2019

@GrahamcOfBorg test matrix-synapse

@ofborg ofborg bot requested a review from pacien October 6, 2019 20:09
@Vskilet
Copy link
Contributor Author

Vskilet commented Oct 6, 2019

Ok pacien I'll create account_threepid_delegates option tomorrow

@risicle
Copy link
Contributor

risicle commented Oct 6, 2019 •
edited

Already broken on darwin, no change here unfortunately :(

(macos 10.13, happy on non-nixos linux x86_64 though)

@gjabell
Copy link
Contributor

gjabell commented Oct 10, 2019

Any movement on this? Setting up a matrix server soon but I'm waiting for 1.4 to be merged first.

@Vskilet
Copy link
Contributor Author

Vskilet commented Oct 10, 2019

I'll finished this tonight or tomorrow morning

@Ma27 Ma27 self-assigned this Oct 11, 2019
@Ma27
Copy link
Member

Ma27 commented Oct 11, 2019

The deprecation breaks eval. I'll have a bit of time this afternoon to fix it :)

Ma27
Ma27 reviewed Oct 11, 2019
View reviewed changes
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

So, just did a review and addressed some minor issues. I deployed the current state onto my personal homeserver to check if I encounter any regressions. @pacien @Vskilet please review.

Regarding my changes:

  • it seems as account_threepid_delegates is simply a block which takes an email and a msisdn value (or am I missing something here?)
  • Dropped the deprecated trusted_third_party_id_servers. People will now get a meaningful error message when using this and upgrading to matrix-synapse 1.4.0.

It would be awesome if the merger of this PR would squash all commits into a single commit with a Co-Authored-by statement for @pacien and me.

@NixOS/backports @Ekleog @pacien I'd propose to backport this even though it contains breaking changes: the old behavior for email activation (which used to rely on trusted_third_party_id_servers) will be disabled on hosted identity-servers (matrix.org, vector.im) in December 2019 (https://matrix.org/blog/2019/10/03/synapse-1-4-0-released), so the old behavior can't be supported after that.

Also, people should notice the change as mkRemovedOptionModule already raises an evaluation error, right?

@ofborg ofborg bot requested a review from Ma27 October 11, 2019 19:14
@worldofpeace
Copy link
Contributor

I'd propose to backport this even though it contains breaking changes: the old behavior for email activation (which used to rely on trusted_third_party_id_servers) will be disabled on hosted identity-servers (matrix.org, vector.im) in December 2019 (https://matrix.org/blog/2019/10/03/synapse-1-4-0-released), so the old behavior can't be supported after that.

Correct @Ma27, if they make a decision like this we unfortunately won't have any choice in the matter.

@infinisil infinisil mentioned this pull request Oct 11, 2019
Merged
@Ekleog
Copy link
Member

Ekleog commented Oct 12, 2019

@Ma27 I didn't have the time to review this yet, so this is neither a r+ or r- and I'll let you handle review, but agree that your arguments appear to me as requiring a backport. Thank you for handling this! :)

@Ma27
Copy link
Member

Ma27 commented Oct 14, 2019

@pacien @Ekleog would you mind taking another look? After that I'd merge/backport :)

@ofborg ofborg bot requested a review from pacien October 14, 2019 21:29
@pacien
Copy link
Contributor

pacien commented Oct 14, 2019

@ofborg test matrix-synapse

pacien
pacien approved these changes Oct 14, 2019
View reviewed changes
Copy link
Contributor

@pacien pacien left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@Vskilet @Ma27
matrix-synapse: 1.3.1 -> 1.4.0
3724223 
Bumps `matrix-synapse` to version 1.4.0[1]. With this version the
following changes in the matrix-synapse module were needed:

* Removed `trusted_third_party_id_servers`: option is marked as deprecated
  and ignored by matrix-synapse[2].
* Added `account_threepid_delegates` options as replacement for 3rdparty
  server features[3].
* Added `redaction_retention_period` option to configure how long
  redacted options should be kept in the database.
* Added `ma27` as maintainer for `matrix-synapse`.

Co-Authored-By: Notkea <pacien@users.noreply.github.com>
Co-authored-by: Maximilian Bosch <maximilian@mbosch.me>

[1] https://matrix.org/blog/2019/10/03/synapse-1-4-0-released
[2] matrix-org/synapse#5875
[3] matrix-org/synapse#5876
@ofborg ofborg bot requested a review from pacien October 14, 2019 23:27
Ma27
Ma27 approved these changes Oct 14, 2019
View reviewed changes
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

Just squashed the commits into a single one as all changes are actually related (all module changes are needed due to the matrix-synapse bump), so bisecting gets noticeable easier that way. Also added @pacien and myself as co-authors.

I'm running those changes with matrix-synapse 1.4.0 on my personal Matrix setup since Friday without encountering any issues. Also tested/reviewed this with @calbrecht this evening.

I'd wait for ofborg to build and merge/backport (to 19.09) then. IMHO it's fine to not backport to 19.03 as those changes are actually a breaking change and 19.03 support will be dropped before the legacy features are disabled on Matrix.org infrastructure.

@GrahamcOfBorg test matrix-synapse

@Ma27 Ma27 merged commit 7774945 into NixOS:master Oct 14, 2019
@Ma27 Ma27 added the 9.needs: port to stable A PR needs a backport to the stable release. label Oct 14, 2019
@Ma27
Copy link
Member

Ma27 commented Oct 14, 2019

Thanks @Vskilet @pacien for the work on this! I just backported this as 1351dde.

@Ma27 Ma27 added 8.has: port to stable A PR already has a backport to the stable release. and removed 9.needs: port to stable A PR needs a backport to the stable release. labels Oct 14, 2019
@Vskilet Vskilet deleted the matrix-synapse-update branch May 1, 2020 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ma27 Ma27 Ma27 approved these changes

@Ekleog Ekleog Awaiting requested review from Ekleog

@Ralith Ralith Awaiting requested review from Ralith

@pacien pacien Awaiting requested review from pacien

Assignees

@Ma27 Ma27

Labels
6.topic: nixos 8.has: module (update) 8.has: port to stable A PR already has a backport to the stable release. 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Vskilet @Ma27 @risicle @gjabell @worldofpeace @Ekleog @pacien