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

Remove legacy db entirely #236

Merged
merged 17 commits into from
Aug 4, 2021
Merged

Conversation

H-Shay
Copy link
Contributor

@H-Shay H-Shay commented Jul 16, 2021

This PR attempts to remove the (hopefully) outdated legacy DB from Sygnal, easing horizontal scaling.

@reivilibre reivilibre self-requested a review July 30, 2021 14:32
Copy link
Contributor

@reivilibre reivilibre 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!

sygnal/gcmpushkin.py Outdated Show resolved Hide resolved
sygnal/gcmpushkin.py Outdated Show resolved Hide resolved
sygnal/sygnal.py Show resolved Hide resolved
tests/testutils.py Outdated Show resolved Hide resolved
@H-Shay
Copy link
Contributor Author

H-Shay commented Aug 2, 2021

I am unsure why the check_types run is failing, this check passes locally...

@H-Shay H-Shay requested a review from reivilibre August 2, 2021 21:41
Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

I think this is close now!

  • please remove sygnal.log :)
  • looks like you've been bitten by changes in Twisted (they added type annotations) which has left you with the trouble of cleaning up mypy after it. Fixing this was very noble of you, but (especially for code which you haven't touched in this PR), it's preferable to do it in a separate PR (this way, we're not waiting on this entire PR being approved before we fix CI [potentially affecting other PRs too]. It also makes reviewing this one a little bit less confusing :)). What we should have done is pin the versions of the dependencies so that this doesn't happen — if you fancy taking that on in another PR (probably using >=...,<... syntax so minor upgrades are still allowed? Refer to Synapse for an example), that would be appreciated and will save the next innocent person from having CI give them trouble :).
  • my mistake for misleading you about the config_setup thing -- we want to reinstate it (since it's still overridden and used by subclasses) & the calls to it from the overriding methods (in case we ever add code to the function) -- just use the word pass in the parent's function, because that's the standard way of saying 'this is intentionally empty' -- you could just as well write 42 or super but it's unusual and confused me into thinking you didn't finish the line 😅 .

sygnal/sygnal.py Outdated Show resolved Hide resolved
tests/testutils.py Outdated Show resolved Hide resolved
sygnal/helper/proxy/connectproxyclient_twisted.py Outdated Show resolved Hide resolved
sygnal/helper/proxy/connectproxyclient_twisted.py Outdated Show resolved Hide resolved
sygnal/helper/proxy/connectproxyclient_twisted.py Outdated Show resolved Hide resolved
tests/test_http.py Outdated Show resolved Hide resolved
tests/test_httpproxy_asyncio.py Outdated Show resolved Hide resolved
tests/test_httpproxy_asyncio.py Outdated Show resolved Hide resolved
tests/test_httpproxy_twisted.py Outdated Show resolved Hide resolved
tests/test_pushgateway_api_v1.py Outdated Show resolved Hide resolved
@H-Shay
Copy link
Contributor Author

H-Shay commented Aug 3, 2021

So I've made all the requested changes, and as such the CI is breaking on the types check, as expected. I opened a separate PR constraining the twisted version and fixing the type errors, perhaps we merge that and then update this branch before merging this branch? Not sure what the best way is!

@H-Shay H-Shay requested a review from reivilibre August 3, 2021 18:14
Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

just a handful of minor changes now, you'll soon see this one merged and released :)

changelog.d/235.misc Outdated Show resolved Hide resolved
sygnal/sygnal.py Outdated Show resolved Hide resolved
sygnal/sygnal.py Outdated Show resolved Hide resolved
sygnal/sygnal.py Outdated Show resolved Hide resolved
@reivilibre
Copy link
Contributor

perhaps we merge that and then update this branch before merging this branch? Not sure what the best way is!

Yup, that'd be fine. I think it's likely that this will be falling into place shortly

@callahad
Copy link
Contributor

callahad commented Aug 4, 2021

...while I'm poking at Sygnal, might as well update this, too ;)

@callahad
Copy link
Contributor

callahad commented Aug 4, 2021

(Note: I'm just doing rote things here, when merging please elide my Signed-off-by: and Co-authored-by: lines)

Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

Great!

@reivilibre reivilibre merged commit d52568e into matrix-org:main Aug 4, 2021
@reivilibre reivilibre mentioned this pull request Aug 9, 2021
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.

3 participants