Skip to content

Conversation

@alexott
Copy link
Contributor

@alexott alexott commented Feb 3, 2022

In current version, we have "catch-all" error handling that efficiently hides the actual reason for the error. It's not possible to understand the reason for error, for example, when IP Access List is used. This patch improves the error reporting:

Without patch

Error: RuntimeError: Can't find repo ID for /Repos/user/repo 

With patch:

Error: RuntimeError: Error fetching repo ID for /Repos/user/repo: HTTP code: 403, Unauthorized access to Org: 1234567890 
@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2022

Codecov Report

Merging #428 (aad60a7) into master (f191338) will decrease coverage by 0.14%.
The diff coverage is 0.00%.

Impacted file tree graph

@@ Coverage Diff @@ ## master #428 +/- ## ========================================== - Coverage 86.21% 86.06% -0.15%  ========================================== Files 41 41 Lines 2887 2892 +5 ========================================== Hits 2489 2489 - Misses 398 403 +5 
Impacted Files Coverage Δ
databricks_cli/repos/api.py 61.76% <0.00%> (-10.65%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f191338...aad60a7. Read the comment docs.

@jaipreet-s
Copy link
Contributor

Changes LGTM, however, it would be good to add unit tests for the new code paths before merging

In current version, we have "catch-all" error handling that efficiently hides the actual reason for the error. It's not possible to understand the reason for error, for example, when IP Access List is used. This patch improves the error reporting: Without patch ``` Error: RuntimeError: Can't find repo ID for /Repos/user/repo ``` With patch: ``` Error: RuntimeError: Error fetching repo ID for /Repos/user/repo: HTTP code: 403, Unauthorized access to Org: 1234567890 ```
@alexott
Copy link
Contributor Author

alexott commented Feb 15, 2022

I've added a test, but I can't see what actual coverage change is

@alexott
Copy link
Contributor Author

alexott commented Mar 16, 2022

@jaipreet-s how I can re-run tests to get new coverage?

Copy link
Contributor

@nfx nfx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@nfx nfx merged commit 4691a89 into databricks:master May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

5 participants