-
Notifications
You must be signed in to change notification settings - Fork 831
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
Process text and binary payloads #1642
Conversation
Hi @Teagan42. Thanks for your PR. I'm waiting for a SeldonIO member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/ok-to-test |
Thanks @Teagan42 |
Tue Mar 31 06:47:49 UTC 2020 impatient try |
Tue Mar 31 06:47:56 UTC 2020 impatient try |
/assign @ryandawsonuk |
/test this |
/retest |
Tue Mar 31 16:51:56 UTC 2020 impatient try |
/check-cla |
/test this |
Looks like the build is failing - have you tested locally with |
Tue Mar 31 18:44:50 UTC 2020 impatient try |
@cliveseldon yes I've used the makefiles to build the JAR and custom image and it is running in my k8s. The link to the logs don't seem to give a lot of info |
@cliveseldon weird, I ran this yesterday fine and now it's failing. I'll look into it. |
@Teagan42 https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-1642/4.log
Which is indeed not clear! Any ideas @axsaucedo |
Tue Mar 31 19:57:42 UTC 2020 impatient try |
Tue Mar 31 19:57:43 UTC 2020 impatient try |
@cliveseldon I'm stupid today. Needed these changes for my job, but we only have enterprise github, so had to mirror it up to our repository and make changes. When I verified it all worked, I forked it from my personal public org and made the changes, but 1. I forgot to update the swagger as I had in the enterprise repo and 2. I missed two imports. Apologies for the brain fart |
@cliveseldon I diagnosed this by adding the |
@Teagan42 Looks great. Is it possible to add a couple of junit tests? seldon-core/engine/src/test/java/io/seldon/engine/api/rest/TestRestClientController.java Lines 311 to 337 in be662dc
|
@cliveseldon Absolutely! I will do that in the morning. I guess I misunderstood the PR instructions as optional (This is a simplified description of our full PR testing and merge workflow that conveniently forgets about the existence of tests, to focus solely on the roles driven by OWNERS files. Please see below for details on how specific aspects of this process may be configured on a per-repo basis.). |
Thu Apr 2 18:54:26 UTC 2020 impatient try |
Thu Apr 2 18:54:36 UTC 2020 impatient try |
@cliveseldon Usually unit tests are a must for me, was in a hurry I guess. But I added tests and everything passes. I may make a PR to update the PR documentation to explicitly state code should be unit tested as the instructions were a little misleading (but that's no excuse for not doing it anyway). |
@Teagan42 Thanks for this! |
/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 |
failed to trigger Pull Request pipeline
|
Convert string and binary payloads to SeldonMessage payloads