-
Notifications
You must be signed in to change notification settings - Fork 46
Add handler for /latest/{platform}[/{test}] urls #166
Conversation
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.
Left some nits
app.yaml
Outdated
@@ -25,6 +25,9 @@ handlers: | |||
- url: /static | |||
static_dir: static | |||
secure: always | |||
- url: /latest/.* | |||
script: _go_app | |||
secure: always |
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.
Adding the route to app.yaml isn't necessary, it'll be caught in the router in main.go.
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.
Done.
latest_test_handler.go
Outdated
// | ||
// URL format: | ||
// /latest/{browser}[/{test}] | ||
func latestTestHandler(w http.ResponseWriter, r *http.Request) { |
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.
I think a more accurate name would be latestSummaryRedirectHandler
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.
Done.
latest_test_handler.go
Outdated
// Assumes that result files are under a directory named SHA[0:10]. | ||
resultsBase := strings.SplitAfter(resultsUrl, "/" + latestRun.Revision)[0]; | ||
resultsPieces := strings.Split(resultsUrl, "/") | ||
re := regexp.MustCompile("(-summary)?\\.json\\.gz$") |
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.
Is this weirdly indented?
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.
Yep - a tab vs spaces. need to tweak my editor settings.
e0e89b1
to
b3ee362
Compare
} | ||
if len(browserPieces) > 3 { | ||
query = query.Filter("OSVersion =", browserPieces[3]) | ||
} |
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 makes some assumptions about the structure of platform IDs that aren't enforced (e.g. firefox-57.0-linux
doesn't have an OS version), but this is fine for now.
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.
Yeah, the 4th chunk could be a problem - I just did this so that /latest/chrome[-63.0[-macos[-10]]]/ all "work".
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 is good to go minus the Go lint errors. Thanks!
b3ee362
to
a1d09f9
Compare
Thanks, will do a deploy today |
e.g.
localhost:8080/latest/chrome-63.0
302's to
https://storage.googleapis.com/wptd/b952881825/chrome-63.0-linux-summary.json.gz
localhost:8080/latest/chrome/2dcontext/building-paths/canvas_complexshapes_arcto_001.htm
302's to
https://storage.googleapis.com/wptd/b952881825/chrome-63.0-linux/2dcontext/building-paths/canvas_complexshapes_arcto_001.htm