Add Seldon PUID Header to gRPC context + tests #1790
Conversation
Signed-off-by: glindsell <[email protected]>
Signed-off-by: glindsell <[email protected]>
|
Mon May 4 13:06:32 UTC 2020 impatient try |
|
Mon May 4 13:08:07 UTC 2020 impatient try |
| g := NewGomegaWithT(t) | ||
| modelName := "foo" | ||
| handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| //g.Expect(r.Header.Get(logger.CloudEventsIdHeader)).Should(Equal(testEventId)) |
| g.Expect(err.Error()).Should(Equal("context value Seldon PUID Header is nil: interface to string conversion failed")) | ||
| } | ||
|
|
||
| func TestModelWithLogResponsesNilPUIDError(t *testing.T) { |
There was a problem hiding this comment.
seems to be almost a duplicate of above test - can't they be combined?
Signed-off-by: glindsell <[email protected]>
|
Tue May 5 10:59:21 UTC 2020 impatient try |
|
Tue May 5 10:59:26 UTC 2020 impatient try |
Signed-off-by: glindsell <[email protected]>
|
Wed May 6 14:54:41 UTC 2020 impatient try |
|
Wed May 6 14:54:51 UTC 2020 impatient try |
|
/retest |
|
Wed May 6 16:12:49 UTC 2020 impatient try |
|
/retest |
|
Wed May 6 16:38:10 UTC 2020 impatient try |
executor/api/grpc/server_test.go
Outdated
| g.Expect(meta[strings.ToLower(payload.SeldonPUIDHeader)]).NotTo(BeNil()) | ||
| g.Expect(meta[strings.ToLower(payload.SeldonPUIDHeader)][0]).To(Equal(guid)) | ||
| g.Expect(meta.Get(payload.SeldonPUIDHeader)).NotTo(BeNil()) | ||
| g.Expect(meta.Get(payload.SeldonPUIDHeader)[0]).To(Equal(guid)) |
There was a problem hiding this comment.
nit: can we fix the typo guid -> puid
| }) | ||
| return nil | ||
| } | ||
|
|
||
| func (p *PredictorProcess) getPUIDHeader() (string, error) { | ||
| // Check request ID is not nil | ||
| if puid, ok := p.Ctx.Value(payload.SeldonPUIDHeader).(string); ok { |
There was a problem hiding this comment.
Will this not still panic if Value return nil? Should we not change for its existence before returning the string?
There was a problem hiding this comment.
Interestingly not...
There's a similar question on this here.
Using the "comma-ok" form:
The value of ok is true if the assertion holds. Otherwise it is false and the value of n is the zero value for type T. No run-time panic occurs in this case.
| graph := &v1.PredictiveUnit{} | ||
| _, err := createPredictorProcessWithoutPUIDInContext(t).Predict(graph, createPredictPayload(g)) | ||
| g.Expect(err).NotTo(BeNil()) | ||
| g.Expect(err.Error()).Should(Equal("context value Seldon PUID Header is nil: interface to string conversion failed")) |
There was a problem hiding this comment.
The message should be turned into a const if we are going to use it in multiple places.
There was a problem hiding this comment.
Fixed. See #1841 for follow up changes
Signed-off-by: glindsell <[email protected]>
|
Mon May 18 13:48:39 UTC 2020 impatient try |
|
Mon May 18 13:48:39 UTC 2020 impatient try |
|
/test integration |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cliveseldon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Mon May 18 13:59:01 UTC 2020 impatient try |
|
/test integration |
|
Tue May 19 12:45:38 UTC 2020 impatient try |
|
/retest |
|
Tue May 19 13:18:40 UTC 2020 impatient try |
|
Tue May 19 15:02:51 UTC 2020 impatient try |
|
Tue May 19 15:03:03 UTC 2020 impatient try |
|
failed to trigger Pull Request pipeline
|
Clean branch of #1782
Fixes #1763