-
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
Support disabling of ssl/tls in seldon_client #3141
Conversation
Hi @juliusvonkohout. Thanks for your PR. I'm waiting for a SeldonIO or todo 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 jenkins-x/lighthouse repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR does not contain suggested fix. Furthermore SSL is not enabled by default - closing |
Just caught up with @cliveseldon - seems there was a discussion on slack to implement this so re-opening. It seems that your branch may be incorrect, could be worth branching from latest master again or rebasing ensuring commits in PR are expected correct ones |
Actually the code is from my colleague @malte2408. I can rebase it against the master but it was written against seldon git master a month ago i guess. I used this file successfully with seldon 1.6. I could also try it with 1.7 release or build the package myself. I guess you are right. E.g feedback_to_json and seldon_message_to_json seem to conflict. So i will rebase. |
f4a18cb
to
8a8b979
Compare
@axsaucedo @cliveseldon Yes, it was I who uploaded the wrong file before. Would you mind to verify the changes again? And we should definitely squash this pull request. |
Can you approve the workflows or tell me which changes are needed? |
/ok-to-test |
@juliusvonkohout You will need to run |
I did exactly that. Please rerun the tests. By the way can this be backported to 1.6? |
So to me this looks mergable. Is it possible to backport this to 1.7 or 1.6 ? |
OK great. Not sure we would want to create bug fix releases for this for 1.7 and 1.6. The client for the next release will work with those releases so it depends how you utilize the Seldon Core software in your org. |
It would be amazing if i can use the 1.8 python package with a seldon 1.6 installation |
Yes you should be able to |
@cliveseldon and @axsaucedo can one of you approve this pull request or do we need someone else? |
/test integration |
Thanks, the integration test ran fine |
Thanks @juliusvonkohout ! |
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #3032
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
@cliveseldon