Page MenuHomePhabricator
Differential D12935

[Tunnelbroker] send push notifs to windows devices
ClosedPublic
Actions

Authored by varun on Jul 30 2024, 12:02 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 2, 2:39 AM
Unknown Object (File)
Sat, Nov 2, 2:38 AM
Unknown Object (File)
Sat, Nov 2, 2:37 AM
Unknown Object (File)
Sat, Nov 2, 2:35 AM
Unknown Object (File)
Sat, Nov 2, 2:35 AM
Unknown Object (File)
Sat, Nov 2, 2:35 AM
Unknown Object (File)
Sat, Nov 2, 2:34 AM
Unknown Object (File)
Sat, Nov 2, 2:33 AM
View All Files
Subscribers
ashoat
tomek

Details

Reviewers
kamil
bartek
marcin
Commits
rCOMM5f3de4311fbb: [Tunnelbroker] send push notifs to windows devices
Summary

Make it possible to send push notifs to windows devices through tunnelbroker

Depends on D12934

Test Plan

https://gist.github.com/vdhanan/3921680cde51e92a8c7787771b36d19a

successfully sent push notifs to my windows desktop app

Diff Detail

Repository
rCOMM Comm
Branch
windows (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

varun created this revision.Jul 30 2024, 12:02 AM
varun held this revision as a draft.
Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 30 2024, 12:02 AM
Harbormaster failed remote builds in B30785: Diff 42928!Jul 30 2024, 12:24 AM
bartek added inline comments.Jul 30 2024, 12:32 AM
shared/tunnelbroker_messages/src/messages/notif.rs
40–41 ↗(On Diff #42928)

See https://linear.app/comm/issue/ENG-8901/fix-serde-parsing-incorrect-tunnelbroker-message-types
The same problem appears here and that's why Commtest CI fails

40–41 ↗(On Diff #42928)

should be TagAwareDeserialize, sorry

varun updated this revision to Diff 42976.Jul 30 2024, 6:55 PM

fix message serialization

Harbormaster completed remote builds in B30823: Diff 42976.Jul 30 2024, 7:12 PM
varun updated this revision to Diff 43207.Aug 6 2024, 8:56 PM

finally got this working and tested end to end

varun published this revision for review.Aug 6 2024, 8:57 PM
Harbormaster completed remote builds in B30973: Diff 43207.Aug 6 2024, 9:12 PM
varun updated this revision to Diff 43213.Aug 6 2024, 9:50 PM
varun retitled this revision from [WIP][Tunnelbroker] send push notifs to windows devices to [Tunnelbroker] send push notifs to windows devices.
varun edited the summary of this revision. (Show Details)
varun edited the test plan for this revision. (Show Details)

update revision

Harbormaster completed remote builds in B30974: Diff 43213.Aug 6 2024, 10:06 PM
bartek accepted this revision.Aug 6 2024, 10:57 PM
bartek added 1 blocking reviewer(s): kamil.

LGTM, letting @kamil take a look at the request format

services/tunnelbroker/src/notifs/wns/mod.rs
40 ↗(On Diff #43213)

nit, to disambiguate WNS token and device token

kamil accepted this revision.Aug 7 2024, 12:42 AM
kamil added inline comments.
services/tunnelbroker/src/notifs/wns/mod.rs
59–69 ↗(On Diff #43213)

Could you create a specific error for status codes?

Similar to what I did in e.g. D12768. Most important are errors about invalid device token which you need for ENG-8913.

71–75 ↗(On Diff #43213)

I don't think this is needed - we don't that for other push services, for keyserver it was implemented because notif ID was needed

This revision is now accepted and ready to land.Aug 7 2024, 12:42 AM
varun updated this revision to Diff 43237.Aug 7 2024, 1:33 PM

address feedback

Harbormaster failed remote builds in B30991: Diff 43237!Aug 7 2024, 1:56 PM
varun updated this revision to Diff 43239.Aug 7 2024, 2:29 PM

address feedback

Harbormaster failed remote builds in B30994: Diff 43239!Aug 7 2024, 2:34 PM
This revision was landed with ongoing or failed builds.Aug 7 2024, 2:41 PM
Closed by commit rCOMM5f3de4311fbb: [Tunnelbroker] send push notifs to windows devices (authored by varun). · Explain Why
This revision was automatically updated to reflect the committed changes.
varun added a commit: rCOMM5f3de4311fbb: [Tunnelbroker] send push notifs to windows devices.

Revision Contents
Changeset List

PathSize
lib/
types/
tunnelbroker/
10 lines
services/
tunnelbroker/
src/
notifs/
4 lines
wns/
4 lines
54 lines
75 lines
websockets/
42 lines
shared/
tunnelbroker_messages/
src/
messages/
1 line
11 lines

Diff 43237

View Options

lib/types/tunnelbroker/notif-types.js

Loading...
View Options

services/tunnelbroker/src/notifs/mod.rs

Loading...
View Options

services/tunnelbroker/src/notifs/wns/error.rs

Loading...
View Options

services/tunnelbroker/src/notifs/wns/mod.rs

Loading...
View Options

services/tunnelbroker/src/notifs/wns/response.rs

Loading...
View Options

services/tunnelbroker/src/websockets/session.rs

Loading...
View Options

shared/tunnelbroker_messages/src/messages/mod.rs

Loading...
View Options

shared/tunnelbroker_messages/src/messages/notif.rs

Loading...