-
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
Remove references to OAuth and the deprecated Seldon OAuth gateway #2623
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Tue Nov 10 16:00:26 UTC 2020 impatient try |
Tue Nov 10 16:01:05 UTC 2020 impatient try |
/test integration |
/test docs |
Tue Nov 10 16:03:13 UTC 2020 impatient try |
Tue Nov 10 16:03:32 UTC 2020 impatient try |
/cc @cliveseldon @RafalSkolasinski @axsaucedo |
Tue Nov 10 16:05:01 UTC 2020 impatient try |
/test integration |
Tue Nov 10 16:24:06 UTC 2020 impatient try |
/test docs |
Wed Nov 11 11:44:37 UTC 2020 impatient try |
Wed Nov 11 11:44:39 UTC 2020 impatient try |
Wed Nov 11 11:45:02 UTC 2020 impatient try |
/test integration |
Wed Nov 11 12:06:14 UTC 2020 impatient try |
/test notebooks |
Wed Nov 11 14:08:38 UTC 2020 impatient try |
@@ -23,7 +23,7 @@ | |||
"source": [ | |||
"## Setup Seldon Core\n", | |||
"\n", | |||
"Use the setup notebook to [Setup Cluster](../../../notebooks/seldon_core_setup.ipynb#Setup-Cluster) with [Ambassador Ingress](../../../notebooks/seldon_core_setup.ipynb#Ambassador) and [Install Seldon Core](../../seldon_core_setup.ipynb#Install-Seldon-Core). Instructions [also online](./seldon_core_setup.html)." |
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 always worry when I see absolute links... One of the issues is that even if someone will be browsing docs for a given version link will lead to latest...
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 agree. Unfortunately, there isn't a better alternative when using nbsphinx-link
.
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 would be worth moving all examples / notebooks inside doc/
folder at some point? Would that help?
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.
That would help, and that's kind of what nbsphinx favours. However, I believe it was decided not to do that to keep the examples folder at the root level of the repo.
Maybe an option worth exploring would be using sym links? Not sure how that would work with Git and nbsphinx.
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.
Symlinks seems reasonable, imo. Maybe worth opening a follow up issue on that.
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.
Good shout. I'll open up an issue.
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.
For reference, this is the issue exploring the symlinks: #2649
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.
Looks good, nice to see code cleanup 👍
I think one more place to adjust in in batch_processor.py
:
CHOICES_GATEWAY_TYPE = ["ambassador", "istio", "seldon"]
probably need to remove seldon
gateway type
My comments are more of a "nitpick" - batch_processor adjustment I am happy to take care in follow up. |
/test notebooks |
@RafalSkolasinski I've kicked off the notebook tests again. Once those pass I think we should be good to merge. |
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RafalSkolasinski 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 Nov 16 15:13:18 UTC 2020 impatient try |
Mon Nov 16 15:13:17 UTC 2020 impatient try |
Mon Nov 16 15:13:21 UTC 2020 impatient try |
Mon Nov 16 15:13:38 UTC 2020 impatient try |
Mon Nov 16 15:16:02 UTC 2020 impatient try |
The
|
/test integration |
Mon Nov 16 16:17:42 UTC 2020 impatient try |
/test integration |
Mon Nov 16 17:37:19 UTC 2020 impatient try |
/test integration |
Tue Nov 17 17:37:31 UTC 2020 impatient try |
/test integration |
Wed Nov 18 09:47:27 UTC 2020 impatient try |
/test integration |
Thu Nov 19 09:38:22 UTC 2020 impatient try |
Thu Nov 19 09:38:43 UTC 2020 impatient try |
@adriangonz: The following tests 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 jenkins-x/lighthouse repository. I understand the commands that are listed here. |
/test integration |
Thu Nov 19 12:05:22 UTC 2020 impatient try |
Thu Nov 19 12:05:40 UTC 2020 impatient try |
Thu Nov 19 12:05:52 UTC 2020 impatient try |
Thu Nov 19 12:05:52 UTC 2020 impatient try |
What this PR does / why we need it:
Remove deprecated OAuth code from
seldon_core
as well as some references to the deprecatedoauth_key
andoauth_secret
fields ofSeldonDeployment
.Which issue(s) this PR fixes:
Fixes #1677
Special notes for your reviewer:
This PR tries to remove most of the references to OAuth and the deprecated Seldon gateway but it's possible that some of them still remain.
There is a
seldon_core.SeldonCallCredentials
object which holds an auth token, but that one seems to have a scope different to the OAuth code (and could still be potentially useful).This PR doesn't remove the
oauth_key
andoauth_secret
fields from the CRD as that would mean breaking changes within the same version.So these fields are still there but flagged as deprecated.
This PR also includes a fix for the
hack/update_python_version.sh
script, which was breaking a couple notebooks (e.g. 9ce35af#diff-df3b13e162caddc01763ecf53f05bc42a0718582e9f5bd3ebb82a558d914e782).Does this PR introduce a user-facing change?: