pigeon
pigeon copied to clipboard
feat: support for Goth modules for FCM
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
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
- [x] module/function docs
- [x] README updates
- Maybe a "how to test without needing a real Goth token" section?
- I have some useful examples but they don't work with the existing tests since they don't use the
Pigeon.Sandbox
adapter.
- I have some useful examples but they don't work with the existing tests since they don't use the
- Maybe a "how to test without needing a real Goth token" section?
Open Questions to discuss
- Should
service_account_json
still be supported?- I think no, but I'm fine with whatever
- Still supporting it would require starting and supervising a
Goth
instance under Pigeon, making config more complicated in my opinion - We probably don't want to continue using the existing
schedule_refresh
approach as it can exhaust the number of tokens you can get per hour depending on how many Dispatcher instances of the same module you have (i.e. multipleYourApp.FCM
instances, but with different names) - it's a "small" change for people to adopt the Goth v1.3.x approach, and better documented by Goth itself
- Should the config key be
:goth
or something else?- maybe
:token_client
, that has a default value, similar to Goth?-
token_client: YourApp.Goth
->{:goth, [YourApp.Goth]}
->Goth.fetch!(YourApp.Goth)
-
token_client: {:goth, [YourApp.Goth, 10_000]
->Goth.fetch!(YourApp.Goth, 10_000)
-
token_client: &YourApp.custom_token_fetch/0
->apply(token_client_fun, [])
-
token_client: {&GothMock.fetch!/1, [YourApp.Goth]}
->apply(token_client_fun, [YourApp.Goth])
-
- maybe
- maybe a MIGRATION.md for the GCP changes?
- Should the FCM tests be split into unit and integration tests?
-
Goth.Token.fetch
documents how to pass ahttp_client
, which makes testing this not too bad - testing locally is a bit onerous, and generally once it gets to the Sandbox, it's likely going to work
- could skip the integration tests by default, and include during CI?
- using a
setup
that doesstart_supervised!
with a custom name per test instead of in test_helper? would make testing locally easier- This might be benefited by updating the macro for
def push
in theDispatcher
__using__
to use the name given to theuse
- Might also benefit to make
push
andchild_spec
defoverridable
- This might be benefited by updating the macro for
-
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.
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
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?
@hpopp any updates on this? would like to use this as well
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
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.
I'll take a look over the week at writing up some docs and seeing if there's anything I missed from discussions
@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.
@hpopp, is there anything else you'd like to see? :)
I just recently started looking at Pigeon V2 in order to get FCM V1 in use. (Project owners missed the warnings about deadlines. :-( ) I have an app that accepts push notifications from two different end user apps to push to Apple and Google for two different mobile apps. Since the mobile apps are different projects I need to be able to use two different configurations. I've just started working on getting them to have two different service account files when I found docs that suggested that Goth can have multiple children, so now I'm wondering if this PR will make it possible for me to have my app, which includes Pigeon which includes Goth, provide the "source" configurations to Goth easily from within my app? The diffs look promising.
I already have my own fork of Pigeon to solve a few other problems. Is there a practical way to get my diffs merged with these ones, or do I desperately need Henry to merge this one?
(One of my problems is that I have an installed base of servers that we can't possibly get upgraded in time, so I'm trying to convert Legacy Notifications to V1 within Pigeon, which I therefore only need executing on my one central app.)
@petermueller You've been working with the Goth, so I hope you will be able to give me a tip or two. I like what you've done so far and I think it will ease my efforts. Since I have the two projects (that I noted above) that I'm sending pushes to, I need to have two different YourAPP.Goth children, and I'm expecting that I will still need them to be using service_account_json configurations.
- Is this the right way to get my AppA requests to use YourApp.GothA and AppB to use YourApp.GothB, all within the new Pigeon Adapter framework? It looks like it would be:
config :your_app, YourApp.FCM.Default,
adapter: Pigeon.FCM,
token_fetcher: YourApp.GothA
config :your_app, YourApp.FCM.Other,
adapter: Pigeon.FCM,
token_fetcher: YourApp.GothB
- Where am I going to configure those local files? Is that in my own config file, using a :pigeon config, or will it use a :goth config? Is that something like:
config :your_app, YourApp.GothA, [source: {:service_account, credentials1}]
config :your_app, YourApp.GothB, [source: {:service_account, credentials2}]
I'm still trying to figure out how to set the credentials correctly from two service files I downloaded, but that's a separate problem. (Although tips are ALWAYS welcome!)
@Sinc63 I believe (1) should be correct. You would just have multiple Pigeon.Dispatchers in your application.ex as well.
(2) the config looks correct assuming you're doing it the way I added in the docs of this PR. Goth is agnostic about how you get your config, it just provides a child_spec. But if you follow the example from the docs I added in this PR you should be good with what you're doing in (2) to support multiple projects. Same as (1), you'd have multiple Goth children, 1 for each project.
With this PR Pigeon would not use the service account json. It's divorced from any token management.
@Sinc63 long term, if you can get your hosts to use the Google metadata server, you can set the default service account for the VM to be a service account that has been granted the appropriate Firebase roles in both projects' IAM pages.
Then you would be able to eliminate the need for the service account json for Goth, also simplifying your build and compile stages.
I do this setup for some of our assets buckets in GCS in different ephemeral projects. It also allows us to use artifact registry cross-project for pulling images and deploying.