codedge-llc / pigeon

iOS and Android push notifications for Elixir
https://hex.pm/packages/pigeon
MIT License
606 stars 133 forks source link

feat: support for Goth modules for FCM #235

Open petermueller opened 1 year ago

petermueller commented 1 year ago

closes #232

Updates the Pigeon.FCM and Pigeon.FCM.Config implementations to delegate the token refresh logic to Goth, as Goth v1.3+ now handles that directly

petermueller commented 1 year ago

We've confirmed that this works with the metadata approach, and have no reason to believe it wouldn't work otherwise. I can test with a service account JSON using Goth if you'd like.

I can also complete any updates to this PR that you'd like. Just let me know what you'd prefer, @hpopp


Original Description:

TODOs

Open Questions to discuss

hpopp commented 1 year ago

This is excellent work 🍻. Considering how long the release candidates have been out, it's not wise to drop :service_account_json, but that also means we can keep the :goth key as-is for custom overrides (with an additional section in the docs, not the initial getting started). One of Pigeon's main goals is having the absolute easiest Getting Started, and in the most common case, users shouldn't even need to be aware it's using Goth. This setup is still simpler than the APNS side, which supports both certs and JWT auth.

For config validation logic, if :goth is present, let that take priority and ignore :service_account_json.

Also, I agree with delegating all refreshing to Goth itself 👍

As far as testing, the integration tests have always been a pain point in PRs. Traditionally I've just pulled locally and run them for a sanity check before merge. I do finally need to split them out from unit tests, but that's a bit too much for this PR.

petermueller commented 1 year ago

Ok so I've had some time to think it over and the best I can think of is having Pigeon fall back to supervising a Goth.PigeonFallback or something like that for the service account json. Only negative is that the only logical place to do that would seem to be in the startup calls for Pigeon.Dispatcher, utilizing maybe a new children callback, or something like that. I don't believe we could do it in the FCM module's init since that's called for each Dispatch Worker. It could be done but would be really finicky and risk causing larger issues if the Worker were restarted as that would kill the linked Goth too.

What do you think of some callback or another function on Configurable or something that can affect the supervision tree in the Dispatcher module? I was thinking Configurable.children that is just an empty list for the other implementations. I can't think of another way to keep the json while getting to delegate the token refresh to Goth. Alternative would be to add a warning if FCM receives the json, and suggest a copy/paste-able chunk of code, but that's not particularly plug and play

dylanseago commented 4 months ago

I'm also looking to use Pigeon with Goth metadata instead of a service account and this looks to be exactly what's needed! Is there a chance this PR will be merged anytime soon or are there more changes needed?

mikeover commented 4 months ago

@hpopp any updates on this? would like to use this as well

petermueller commented 4 months ago

Hey, I apologize I forgot about this PR. Let me know if you want me to take a stab at anything. I recently took on co-maintainership of waffle_gcs but I've been generally carving out more time for open source contributions on the weekends

hpopp commented 3 months ago

Finally getting a chance to catch back up to speed, I've been traveling a lot lately. I think I'm coming around on dropping the service_account_json key. Considering that legacy FCM is getting shut off in June of this year, longtime users of this package are going to have to revise their setup and upgrade anyway (Pigeon 3.0 anyone?). Let's get some setup docs behind this and consider it the last major change for the 2.0 release.

petermueller commented 3 months ago

I'll take a look over the week at writing up some docs and seeing if there's anything I missed from discussions

petermueller commented 3 months ago

@hpopp, let me know what you think. I can move stuff around, or if there's preferred word-smithing, I take no offense 😅

Final question I had though was whether we should provide a deprecation warning (and whether hard at compile, or just at runtime) if they provide :service_account_json. I left in the redact so whatever we decide I don't think it will leak their secrets to logs.

petermueller commented 2 months ago

@hpopp, is there anything else you'd like to see? :)