search

matrix-org / sygnal

Sygnal: reference Push Gateway for Matrix
Apache License 2.0
160 stars 144 forks source link

Clarify use of 'FCM' in app type and refactor to use 'FCM' #251

Closed H-Shay closed 1 month ago

H-Shay commented 3 years ago

Addresses issue #230. This PR allows for both gcm and fcm to be used as app types, warns that the app type gcm will soon be depreciated, and refactors sygnal to clarify that we are using Firebase Cloud Messaging exclusively.

H-Shay commented 3 years ago

Totally baffled as to why the changelog check is failing.

reivilibre commented 3 years ago

the changelog issue is odd. It was shown to work in the PR that added it :|. I can't see what's wrong with it or why it would act differently in either case.

reivilibre commented 3 years ago

it seems like there's an issue with the newsfile checker when used with a fork rather than a branch, as I found out by experimenting with #253 (and #252). I'll look into this, sorry for the hassle (edit: #254 should address this)

callahad commented 3 years ago

Without looking at this too closely, do we have opinions about referring to this as "FCM" versus something more specific to the FCM API we implement?

Namely, Sygnal supports the "legacy HTTP" API, not the newer "HTTP v1" API (cite: Migrate from legacy HTTP to HTTP v1)

Concretely this would probably mean using fcm_legacy or similar instead of just fcm so that we have room for fcm_v1 later.

reivilibre commented 3 years ago

Without looking at this too closely, do we have opinions about referring to this as "FCM" versus something more specific to the FCM API we implement?

Namely, Sygnal supports the "legacy HTTP" API, not the newer "HTTP v1" API (cite: Migrate from legacy HTTP to HTTP v1)

Concretely this would probably mean using fcm_legacy or similar instead of just fcm so that we have room for fcm_v1 later.

On one hand, I don't see much advantage, unless we intend to support both separately? They may be two different APIs, but they are both APIs onto the same service. We never had an app type change when we moved over from the old, binary-protocol APNS to the HTTP/2 APNS, for example. Renaming it to something with 'legacy' in the name is kind of unfortunate when it's the 'gcm' option that's the legacy option.

On the other hand, perhaps we will migrate to the v1 API one day (if there's a use case for it). I can't tell if admins will need to make any config changes then or not (it seems like the format of access tokens might have changed, so perhaps?). If that's the case, I can feel someone wanting to say that we should honour both options (:P) for some time whilst admins migrate (although I suppose nobody is forcing them to upgrade Sygnal until they're ready to amend the configuration).

anoadragon453 commented 3 years ago

Having fcm_legacy without a fcm sounds a bit confusing. I'd suggest we either just do fcm_v1 when the time comes, or simply add an additional option under the pushkin config to choose the API, while still keeping an app type of fcm.

rltas commented 1 year ago

What about supporting both type: gcm and type: fcm and introducing a new parameter server_key for the latter? Then for a transition period, existing configs keep working with the legacy API without config changes: type: gcm api_key: your_api_key_for_gcm But if you're going with fcm, it would have to be: type: fcm server_key: your_server_key_for_gcm This would be more proper (less confusing) naming anyway. Then when #313 is ready, issue deprecation warnings on startup for a transition period when server_key is used, before finally removing the old code altogether, because Google will remove the legacy API at some point. This would be a good process for those who have to maintain Sygnal (admins), because they're usually not the ones doing the Google/app stuff. Because this change has been delayed for quite some time, while Matrix gained quite a bit of popularity, many new setups unfortunately now use type: gcm and have to be given some head notice. When using the ansible playbook to setup a Matrix stack, this can be elegantly mitigated anyway.