Paths

Table of Contentst

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

	F3180337: D12935.id42976.diff
	Fri, Nov 8, 4:45 AM

	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

Subscribers

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

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	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	should be `TagAwareDeserialize`, sorry

fix message serialization

Harbormaster completed remote builds in B30823: Diff 42976.Jul 30 2024, 7:12 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

update revision

Harbormaster completed remote builds in B30974: Diff 43213.Aug 6 2024, 10:06 PM

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

address feedback

Harbormaster failed remote builds in B30991: Diff 43237!Aug 7 2024, 1:56 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

Path

Size

services/

tunnelbroker/

src/

notifs/

wns/

14 lines

54 lines

websockets/

39 lines

shared/

tunnelbroker_messages/

src/

messages/

1 line

11 lines

Diff 42928

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

Loading...

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

Loading...

services/tunnelbroker/src/websockets/session.rs

Loading...

shared/tunnelbroker_messages/src/messages/mod.rs

Loading...

shared/tunnelbroker_messages/src/messages/notif.rs

Loading...