-
Notifications
You must be signed in to change notification settings - Fork 835
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow REST error payloads to be returned #1446
Conversation
Sat Feb 15 10:07:37 UTC 2020 impatient try |
Sat Feb 15 10:07:43 UTC 2020 impatient try |
/test this |
Sat Feb 15 10:18:45 UTC 2020 impatient try |
executor/api/rest/client.go
Outdated
@@ -163,7 +158,13 @@ func (smc *JSONRestClient) doHttp(ctx context.Context, modelName string, method | |||
|
|||
contentType := response.Header.Get("Content-Type") | |||
|
|||
return b, contentType, nil | |||
err = nil |
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.
err should already be nil if code has reached this point, why this nil re-assignment?
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.
Can line 161 be removed?
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 approved right before your comment so I've added a /hold
to block the merge. Feel free to remove it once you are happy @glindsell!
executor/api/rest/client_test.go
Outdated
w.WriteHeader(http.StatusInternalServerError) | ||
w.Write([]byte(errorPredictResponse)) | ||
}) | ||
host, port, httpClient, teardown := testingHTTPClient(g, h) |
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.
Gomega has its own package for testing http clients ghttp
: https://onsi.github.io/gomega/#ghttp-testing-http-clients
Might be worth looking into seeing as we're using it for assertions.
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 ghttp
is not compatible with NewGomegaWithT
yet (onsi/gomega#321) as it's mainly designed to be used with gingkgo
. However, given that we already have some manual teardown()
, it could make sense to start using ginkgo
(and ghttp
).
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.
Maybe we create an issue for this?
w.Write([]byte(errorPredictResponse)) | ||
}) | ||
host, port, httpClient, teardown := testingHTTPClient(g, h) | ||
defer teardown() |
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.
Gomega's BeforeEach and AfterEach are another option for how to manage test setup and teardown
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.
As its not for all test might leave for now?
Tue Mar 3 09:11:04 UTC 2020 impatient try |
Tue Mar 3 09:11:08 UTC 2020 impatient try |
Tue Mar 3 09:17:07 UTC 2020 impatient try |
Tue Mar 3 09:18:17 UTC 2020 impatient try |
/approve |
Fri Mar 6 11:45:25 UTC 2020 impatient try |
Fri Mar 6 11:45:27 UTC 2020 impatient try |
/hold |
/test integration |
Wed Mar 11 18:51:16 UTC 2020 impatient try |
Wed Mar 11 18:51:24 UTC 2020 impatient try |
Wed Mar 11 18:53:06 UTC 2020 impatient try |
@cliveseldon: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
There was an error on the pre-packaged servers, I've never seen that one before. seldon-core/jenkins-x/logs/SeldonIO/seldon-core/PR-1446/14.log Lines 1361 to 1396 in a7a5684
|
Fri Mar 13 07:58:01 UTC 2020 impatient try |
Fri Mar 13 07:59:25 UTC 2020 impatient try |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adriangonz, glindsell 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 |
Fixes #939