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

modify push notification settings + use fcm-django library #989

Closed
wants to merge 14 commits into from

Conversation

joeyorlando
Copy link
Contributor

@joeyorlando joeyorlando commented Dec 14, 2022

What this PR does

  • swaps out django-push-notifications for fcm-django. Again.. this is a fork of the parent repo for exactly the same reason.. the migrations point to auth_user without letting us use our own user model, this has been patched in the grafana fork. The reason why we are using fcm-django vs django-push-notifications is that the latter does not support the new FCM API, only the "legacy" API. The legacy FCM API does not support certain push notification settings that we would like to use.
  • modifies the iOS/Android specific push notification settings
  • adds a flower pod in the docker-compose-developer.yml, useful for debugging tasks locally
  • sets the mobile app verification token TTL to 5 minutes when developing locally. The default of 1 minute makes working with device emulators really tricky..

Other stuff

This PR also swaps out the base image in engine/Dockerfile from python:3.9-alpine3.16 to python:3.9-slim-buster.

As to why.. in short, with the introduction of the fcm-django library there is now a peer-dependency on grpcio (which is used by firebase_admin.. which I am using in this PR to interact directly with Firebase Cloud Messaging (FCM)). grpcio does not publish wheels (read: compiled binaries) for the Alpine distro. It does publish wheels for Debian and hence pip install -r requirements.txt does not need to build this library from the source distribution.

This is a known "issue" and the recommended solution in the community is to.. not use alpine.

These were the numbers, when building the image locally, in terms of image size and build time:

Local image size (uncompressed Build time
python:3.9-alpine3.16 785MB 320s
python:3.9-slim-buster 1.05GB 90s

@joeyorlando joeyorlando changed the title modify push notification settings modify push notification settings + use fcm-django library Dec 14, 2022
@joeyorlando joeyorlando marked this pull request as ready for review December 14, 2022 14:01
@joeyorlando joeyorlando requested review from a team as code owners December 14, 2022 14:01
"alert": message,
"sound": "bingbong.aiff",
}
# TODO: we should update this to check if FCM_RELAY is set and conditionally make a call here..
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right now the fcm_relay endpoint won't work properly, but we are not using this yet. Let's come back in a follow-up PR and patch this.

@joeyorlando
Copy link
Contributor Author

note: once this is deployed we should drop the tables associated with the push_notifications django app as they're no longer needed

@CLAassistant
Copy link

CLAassistant commented Dec 14, 2022

CLA assistant check
All committers have signed the CLA.

@@ -33,7 +33,7 @@ django-log-request-id==1.6.0
django-polymorphic==3.0.0
django-rest-polymorphic==0.1.9
pre-commit==2.15.0
https://github.com/grafana/django-push-notifications/archive/refs/tags/3.0.0-fix-migration.tar.gz
https://github.com/grafana/fcm-django/archive/refs/tags/v1.0.12r1.tar.gz
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I again needed to make a fork because fcm-django had the same issue as django-push-notifications in that it wouldn't properly allow specifying the foreign key relationship for the user_id column (would automatically set this to auth_user.. which isn't appropriate here). The forked version has this issue patched.

to "FCM" no matter what was provided in the request.
"""

cloud_message_type = HiddenField(default="FCM")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is no longer necessary because fcm_dango only works with.. FCM and thereby doesn't have a cloud_message_type field

"status": alert_group.status,
"aps": aps,
}
message = Message(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was the main focus of this PR, adjusting the push notification settings to include some iOS and Android specific things. django-push-notifications didn't allow this because it was using the "legacy" FCM API whereas fcm-django uses the latest API, which supports these features.

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.

None yet

3 participants