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: key not from file & other improvements #6

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

chris13524
Copy link

This adds support for reading the service account key from a ServiceAccountKey rather than the default using the dotenv variable. Also:

  • Adds a default feature flag for the dotenv dependency and functionality for Client::new(). new() now returns a Result since loading the key is performed immediately rather than later. Remove Default implementation since it doesn't support errors
  • Add ClientBuilder pattern which enables more complex constructors such as allowing to provide a custom reqwest HTTP client
  • Rename and re-export types in client::response to enable callers to match on the error variants. Re-export gauth to enable access to ServiceAccountKey
  • Share the HTTP client with gauth
  • Pull project_id from ServiceAccountKey rather than double-parsing the value
  • Remove .pool_max_idle_per_host(usize::MAX) configuration on reqwest since this is the default
  • Take advantage of gauth now returning bearer token directly for simplified code
  • Switch to .json() helper from reqwest to construct JSON request bodies over doing this by hand with .body(), Content-Type header, and serde
  • Resolves various clippy and fmt errors

Resolves #5

Depends on makarski/gauth-rs#29

Example usage in the project I'm working on: WalletConnect/push-server#316

@chris13524 chris13524 changed the title feat: key not from file feat: key not from file & other improvements Apr 18, 2024
@chris13524 chris13524 marked this pull request as ready for review April 19, 2024 12:44
@chris13524 chris13524 marked this pull request as draft April 19, 2024 12:44
@chris13524 chris13524 marked this pull request as ready for review April 19, 2024 16:31
@rj76
Copy link
Owner

rj76 commented Apr 20, 2024

Looking good!

Thank you for this. Shall we merge once your PR in gauth-rs is merged and published?

We might also want this fork published, but I haven't done that before and I'm not sure about a proper crate name... I'm also not particularly comfortable with being the only maintainer or owner :)

@chris13524
Copy link
Author

chris13524 commented Apr 23, 2024

Yeah let's wait for the gauth-rs PR to be merged before merging this. I'm working on a few more tweaks/fixes and I'd also like to test all this out in prod on my end first to make sure everything is solid.

I think we should also change the crate version from 1.0.0 to 0.10.0 since this PR makes breaking changes, and there could be more breaking changes going forward. Not sure it's a good idea to commit to a stable API quite yet.

Regarding publishing the fork, it's possible you could get ownership of the crate name from the previous maintainer since that repo appears to no longer be maintained and the current version of FCM it uses will no longer work past June. Alternatively fcm_v1 might be a fine name.

I'm happy to help maintain; my company also maintains the a2 library for APNs.

@chris13524 chris13524 marked this pull request as draft May 13, 2024 18:15
@rj76
Copy link
Owner

rj76 commented May 22, 2024

Could you also have a look at #10? The yup-oauth2 package is well maintained and also has what you need, if I'm correct.

@chris13524
Copy link
Author

chris13524 commented May 22, 2024

Could you also have a look at #10? The yup-oauth2 package is well maintained and also has what you need, if I'm correct.

It doesn't look like that PR provides a way to get a service account key from a string, also looks like there will be a lot of merge conflicts here.

Perhaps we should drop gauth and switch to yup-oauth2?

@rj76
Copy link
Owner

rj76 commented May 22, 2024

Perhaps we should drop gauth and switch to yup-oauth2?

Yeah, that was also what I was thinking, but wanted to check with you because of this PR.

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.

Support for multi-tenancy
2 participants