fix: Improve the error message when Application Default Credentials are not configured #173
Conversation
Summary of ChangesHello @tim-u, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness and user-friendliness of the Dataproc Spark Connect client by introducing specific error handling for cases where Google Cloud default credentials are not configured. By catching Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the error handling for missing user default credentials by adding a specific except block for DefaultCredentialsError. This provides a clearer error message to the user. A corresponding unit test has been added to verify this behavior.
My review includes two main points:
- In
google/cloud/dataproc_spark_connect/session.py, there's an opportunity to refactor the exception handling logic to reduce code duplication acrossexceptblocks. - In
tests/unit/test_session.py, the new test case can be written more idiomatically using theassertRaisescontext manager for better readability.
Overall, the changes are good and improve user experience. Addressing the feedback will enhance code maintainability and test clarity.
| stop_create_session_pbar_event.set() | ||
| if create_session_pbar_thread.is_alive(): | ||
| create_session_pbar_thread.join() | ||
| DataprocSparkSession._active_s8s_session_id = None | ||
| DataprocSparkSession._active_session_uses_custom_id = False |
There was a problem hiding this comment.
This cleanup logic is duplicated in three different except blocks (this one, the one for InvalidArgument/PermissionDenied, and the generic Exception one). To improve maintainability and reduce redundancy, consider factoring this logic out. You could, for example, use a single except Exception as e: block that contains the cleanup logic and then uses isinstance checks to raise the appropriate specific exception.
| try: | ||
| DataprocSparkSession.builder.location("test-region").projectId( | ||
| "test-project" | ||
| ).getOrCreate() | ||
| except DataprocSparkConnectException as e: | ||
| self.assertIn("Your default credentials were not found", str(e)) |
There was a problem hiding this comment.
For better readability and to follow common testing patterns, consider using the assertRaises context manager to test for exceptions. This makes the intent of the test clearer.
with self.assertRaises(DataprocSparkConnectException) as e:
DataprocSparkSession.builder.location("test-region").projectId(
"test-project"
).getOrCreate()
self.assertIn("Your default credentials were not found", str(e.exception))…dentials-error' into improve-missing-credentials-error
medb
changed the title
| DataprocSparkSession._active_s8s_session_id = None | ||
| DataprocSparkSession._active_session_uses_custom_id = False | ||
| raise DataprocSparkConnectException( | ||
| f"Error while creating Dataproc Session: {e}" |
There was a problem hiding this comment.
We probably need to add a link to the ADC docs so users can fix it up more easily: https://docs.cloud.google.com/docs/authentication/provide-credentials-adc
| raise DataprocSparkConnectException( | ||
| f"Error while creating Dataproc Session: {e.message}" | ||
| ) | ||
| except DefaultCredentialsError as e: |
There was a problem hiding this comment.
Did we check whether this will fix also #142 issue?
medb
changed the title
2 participants
Fixes #142