-
Notifications
You must be signed in to change notification settings - Fork 9
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: FCM v1 #316
feat: FCM v1 #316
Conversation
@@ -81,7 +81,7 @@ jobs: | |||
id: tf-apply | |||
uses: WalletConnect/actions/terraform/apply/@1.0.3 | |||
env: | |||
GRAFANA_AUTH: ${{ steps.grafana-get-key.outputs.key }} | |||
TF_VAR_grafana_auth: ${{ steps.grafana-get-key.outputs.key }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoids this error in newer TF versions and matches other repos:
╷
│ Error: Missing required argument
│
│ with provider["registry.terraform.io/grafana/grafana"],
│ on provider.tf line 16, in provider "grafana":
│ 16: provider "grafana" {
│
│ "auth": one of `auth,cloud_api_key,oncall_access_token,sm_access_token` must be specified
╵
╷
│ Error: Missing required argument
│
│ with provider["registry.terraform.io/grafana/grafana"],
│ on provider.tf line 17, in provider "grafana":
│ 17: url = "https://${var.grafana_endpoint}"
│
│ "url": all of `auth,url` must be specified
╵
error: Recipe `tf-validate` failed with exit code 1
Show Plan
Action: |
Show Plan
Action: |
Show Plan
Action: |
Show Plan
Action: |
Show Plan
Action: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it's a draft it looks good to me! Thanks for updating to v1 👍
Show Plan
Action: |
Show Plan
Action: |
@@ -16,12 +16,10 @@ pub struct DecryptedPayloadBlob { | |||
} | |||
|
|||
impl DecryptedPayloadBlob { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused function and improve error specificity
Show Plan
Action: |
@@ -85,7 +85,7 @@ jobs: | |||
rustc: nightly | |||
- name: "Unit Tests" | |||
cmd: test | |||
args: --features multitenant,analytics,geoblock,functional_tests | |||
args: --features multitenant,analytics,geoblock,functional_tests,apns_tests,fcm_tests,fcmv1_tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made into separate features to enable running tests locally without all of these test secrets.
fcm_options: None, | ||
direct_boot_ok: None, | ||
}), | ||
// TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file missing a lot of options; will be corrected in follow-up PR
Show Plan
Action: |
Show Plan
Action: |
Description
Adds support for the new FCM API using a fork of the current FCM crate: rj76/fcm-rust
Resolves #294
Remaining work:
ECHO_TEST_FCM_V1_CREDENTIALS
secretupdate_fcm_v1
routeEnumerate and handle error cases:[FCM v1] Better error handling #321Key parse as RSAAccess token status codeUnauthorized (?) on FCM sendmatch result
by exporting necessary fieldsAdd length constraints forAdd length constraints for fields #322fcm_v1_credentials
,fcm_api_key
,apns_topic
,apns_certificate
, andapns_certificate_password
,apns_key_id
,apns_team_id
,apns_pkcs8_pem
Complete full FCM payload w/ fallbacks i.e. Android, web pushhttps://github.com/WalletConnect/push-server/issues/newTo ship:#324Work with Cloud team to add ability to configure new credentials and add necessary endpoints to Push ServerConsider Cloud-facing API: shouldenabled_providers
include bothfcm
andfcm_v1
? Should there be a separate endpointupdate_fcm_v1
to update? Right now yesInform users to setup new credentialsHow Has This Been Tested?
TBD
Due Diligence