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

feat: support for Goth modules for FCM #235

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

petermueller
Copy link

@petermueller petermueller commented Feb 15, 2023

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
Copy link
Author

petermueller commented Feb 15, 2023

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

  • module/function docs
  • 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.

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. multiple YourApp.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 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 a http_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 does start_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 the Dispatcher __using__ to use the name given to the use
      • Might also benefit to make push and child_spec defoverridable

@hpopp
Copy link
Member

hpopp commented Feb 18, 2023

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
Copy link
Author

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
Copy link

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
Copy link

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

@petermueller
Copy link
Author

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
Copy link
Member

hpopp commented Mar 16, 2024

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
Copy link
Author

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

@petermueller petermueller force-pushed the pkm/support-gcp-default-application-credentials branch from dbb0da5 to e721c10 Compare April 1, 2024 02:35
@petermueller
Copy link
Author

@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
Copy link
Author

petermueller commented Apr 9, 2024

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

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.

Pigeon.FCM config, service_account_json, goth, and Google's Application Default Dredentials changes
4 participants