-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Expose authenticate_user
#6485
Expose authenticate_user
#6485
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
…eks/Cirq into u/smeeks/add_auth_to_get_engine
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6485 +/- ##
=======================================
Coverage 97.76% 97.76%
=======================================
Files 1105 1105
Lines 94936 94957 +21
=======================================
+ Hits 92810 92831 +21
Misses 2126 2126 ☔ View full report in Codecov by Sentry. |
return get_engine(project_id) | ||
except Exception as exc: | ||
print( | ||
"Authentication failed, you may not have permission to access a" |
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.
Should the "authentication failed" message occur en the authentication step? e.g., what happens if get_engine
raises an exception?
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.
n/a since this api was removed
@@ -152,3 +134,68 @@ def get_qcs_objects_for_notebook( | |||
processor_id=processor_id, | |||
is_simulator=is_simulator, | |||
) | |||
|
|||
|
|||
def get_hardware_engine_and_authenticate_user( |
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.
The long name makes this function harder to reason about and calling it as just as long as calling the constituent parts. WYDT of exposing authenticate_user
and calling that from the notebook instead?
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.
+1, updated to make authenticate_user
public
get_hardware_engine_and_authenticate_user
authenticate_user
Create and expose new
authenticate_user
apiCurrently an API does not exist that returns an engine instance and does authentication.
get_engine
assumes the user is in the corp network and will fail for external users without access to corp. (as noted in #6483)