Add results receiving API to Go webapp #142
Conversation
|
Manual staging deployment: https://robertma-dev-dot-wptdashboard.appspot.com/api/results/upload (need to login as admin) |
webapp/queue.yaml
Outdated
| queue: | ||
| - name: results-arrival | ||
| target: processor | ||
| rate: 1/m |
There was a problem hiding this comment.
Why 1 per min? Is there an impact of having it more often / less often?
There was a problem hiding this comment.
Oops, I didn't mean to set the rate. It's the max_concurrent_requests that I wanted to change because I use manual scaling for the Flex service.
Removed rate and added max_concurrent_requests (set to 1 for now).
| req := httptest.NewRequest("GET", "/api/results/upload", new(strings.Reader)) | ||
| resp := httptest.NewRecorder() | ||
| mockAE := NewMockAppEngineAPI(mockCtrl) | ||
| mockAE.EXPECT().login("/api/results/upload").Return(false, "http://wpt.fyi/login") |
There was a problem hiding this comment.
"/login" would be more environment-independent.
api/receiver/results_receiver.go
Outdated
| <label> result_url <input name="result_url"></label><br> | ||
| <input type="submit"> | ||
| </form> | ||
| </html>` |
There was a problem hiding this comment.
Put the HTML in a template file (e.g. /templates/admin/upload)
There was a problem hiding this comment.
@lukebjerring I'm having path issues, different CWD in unit tests vs. prod/dev_appserver. (similar to what you saw when you were playing with browsers.json). How can I work around this?
There was a problem hiding this comment.
Discussed with @lukebjerring offline. We didn't really find a good solution for the path issue per se, but we decided to move this page to webapp.
| t, err = handleFilePayload(a, uploader, f) | ||
| } | ||
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) |
There was a problem hiding this comment.
Need to return after erroring.
api/receiver/results_receiver.go
Outdated
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| } | ||
| fmt.Printf("Task %s added to queue\n", t.Name) |
There was a problem hiding this comment.
Write this to the response?
api/receiver/gcs.go
Outdated
| return w, nil | ||
| } | ||
|
|
||
| var _ gcs = (*gcsImpl)(nil) |
api/receiver/appengine_test.go
Outdated
| assert.Nil(t, err) | ||
|
|
||
| assert.Equal(t, path, fmt.Sprintf("/%s/test.json", BufferBucket)) | ||
| assert.Equal(t, bytes.Compare(mGcs.mockWriter.finalContent, []byte("test content")), 0) |
There was a problem hiding this comment.
I think assert.Equal(t, string(mGcs.mockWriter.finalContent), "test content") will give you a more readable output if a regression occurs.
api/receiver/appengine.go
Outdated
| } | ||
| } | ||
|
|
||
| func (a *appEngineAPIImpl) login(url string) (loggedIn bool, loginURL string) { |
There was a problem hiding this comment.
I find this API strange, because it doesn't actually log anyone in (even via side-effects of user.Current).
Maybe have 2 methods, isLoggedIn, and getLoginUrl(redirect string) ?
api/receiver/appengine.go
Outdated
| isAdmin() bool | ||
| login(url string) (loggedIn bool, loginURL string) | ||
| uploadToGCS(fileName string, f io.Reader, gzipped bool) (gcsPath string, err error) | ||
| scheduleResultsTask(uploader string, gcsPaths []string, payloadType string) (*taskqueue.Task, error) |
There was a problem hiding this comment.
taskqueue.Task is a struct, not an interface. Should it be abstracted for testing?
There was a problem hiding this comment.
AFAIK it doesn't have any exported methods. And all we need are its exported fields. I don't think we can abstract fields using interface, can we?
There are two reasons: 1. Referencing to webapp/templates from other packages (directories) is no fun because of the changing CWD during unit testing. 2. Since the admin upload page isn't really an API, it makes more logical sense to put it in webapp.
|
After discussion the admin-only upload page is now moved to Rationales:
@lukebjerring PTAL |
This is the receiver part of the results receiver API (sorry for the confusing name), which is implemented inside the default Go app.
This PR implements almost everything in the design doc -- except the authentication of the uploaders, which will be added in a follow-up PR.
Tracking issue: #55