search

matrix-org / sygnal

Sygnal: reference Push Gateway for Matrix
Apache License 2.0
164 stars 147 forks source link

FCM v1: use async version of google-auth and add HTTP proxy support #372

Closed MatMaul closed 4 months ago

MatMaul commented 4 months ago

Fix #371.

MatMaul commented 4 months ago

Another kick is required to trigger the workflows, this is annoying. @devonh if you can trigger them again please :pray: .

MatMaul commented 4 months ago

Thanks for the fast review.

I've address the comments and also differentiate the error cases for service_account_file between not specified and invalid file, so we can surface the underlying load error.

MatMaul commented 4 months ago

Also perhaps an useful info, this is deployed in our preprod (with an HTTP proxy) with no trouble.

devonh commented 4 months ago

This is looking good.

Would you be able to add a couple of unit tests? Particularly around loading the service account file and _get_auth_header(). (patching out the call to credentials.refresh)

Once those are in place I am comfortable with merging this.

MatMaul commented 4 months ago

I've added a fake service account file to test loading, and create a FakeCredentials to exercise _get_auth_header.

Unfortunately refresh is never called and is blocking, for some async magic I don't really understand between Twisted and asyncio I believe. I am a bit stuck so I pushed the changes in case someone has an idea.

devonh commented 4 months ago

I've added a fake service account file to test loading, and create a FakeCredentials to exercise _get_auth_header.

Unfortunately refresh is never called and is blocking, for some async magic I don't really understand between Twisted and asyncio I believe. I am a bit stuck so I pushed the changes in case someone has an idea.

Thank you for adding the tests. I'll take a look at the blocking issue today to see if I can figure anything out with that.