Publish system events on space prefixed path for hosted version #485
Conversation
Codecov Report
@@ Coverage Diff @@
## master #485 +/- ##
==========================================
- Coverage 70.78% 70.77% -0.02%
==========================================
Files 35 37 +2
Lines 2184 2190 +6
==========================================
+ Hits 1546 1550 +4
- Misses 574 576 +2
Partials 64 64
Continue to review full report at Codecov.
|
router/path_hosted.go
Outdated
|
|
||
| // systemPathFromPath constructs path from path on which event was emitted. Helpful for "event.received" system event. | ||
| func systemPathFromPath(path string) string { | ||
| return "/" + strings.Split(path, "/")[1] + "/" |
There was a problem hiding this comment.
We guarantee here somehow that path would never be / only, correct? Otherwise this will blow up during subindexing.
There was a problem hiding this comment.
Yes, this code is only compiled for hosted EG. We guarantee that the first segment of path is always space name. Also if that assumption is wrong it's actually better to blow up asap :)
router/path.go
Outdated
| } | ||
|
|
||
| func systemPathFromPath(space string) string { | ||
| return "/" |
There was a problem hiding this comment.
Make / into a keyword somewhere in router package, something like BASEPATH? We do use it a lot throughout.
There was a problem hiding this comment.
Yeah, sure I can do that.
There was a problem hiding this comment.
Do you think that this should be used only in this file or also in path_hosted.go?
There was a problem hiding this comment.
You can make the global const available package-wide and just user wherever you need!
| @@ -458,7 +454,7 @@ func (router *Router) emitSystemEventReceived(path string, event eventpkg.Event, | |||
| mimeJSON, | |||
| eventpkg.SystemEventReceivedData{Path: path, Event: event, Headers: ihttp.FlattenHeader(header)}, | |||
| ) | |||
| router.handleAsyncSubscriptions(http.MethodPost, "/", *system, nil) | |||
| router.handleAsyncSubscriptions(http.MethodPost, systemPathFromPath(path), *system, nil) | |||
There was a problem hiding this comment.
I think this would be more idiomatic to use something like BASEPATH instead of the helper function call.
There was a problem hiding this comment.
The reason for helper function is that systemPathFromPath is compiled differently depending on build tag. That's the reason for having two implementations for those three functions
There was a problem hiding this comment.
Understood, that makes a lot of sense
|
|
||
| # include coverage for hosted EG | ||
| go test -race -coverprofile=profile.out -covermode=atomic -tags=hosted ./router |
There was a problem hiding this comment.
Can't we put the coverprofile straight to coverage.txt?
codecov.sh
Outdated
|
|
||
| # include coverage for hosted EG | ||
| go test -race -coverprofile=profile.out -covermode=atomic -tags=hosted ./router >> coverage.txt |
There was a problem hiding this comment.
What I was saying was more like...
go test --reace --coverprofile=coverage.txt --covermode=atomic --tags=hosted ./router
Todos:
Is this ready for review?: Yes
Is it a breaking change?: NO