Skip to content
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

Patch dbutils.notebook.entry_point... to return current local notebook path from env var #618

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

kartikgupta-db
Copy link
Contributor

@kartikgupta-db kartikgupta-db commented Apr 17, 2024

Changes

Proposal

  • Allow users to add their own patches to make is easier for users using dbutils from sdk to workaround such issues in the future.

Tests

  • Unit tests
  • make test run locally
  • make fmt applied
  • relevant integration tests applied

Copy link

github-actions bot commented Apr 17, 2024

This PR breaks backwards compatibility for databrickslabs/ucx downstream. See build logs for more details.

Running from downstreams #81

@codecov-commenter
Copy link

codecov-commenter commented Apr 17, 2024

Codecov Report

Attention: Patch coverage is 87.75510% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 57.74%. Comparing base (a1055d1) to head (f914957).
Report is 42 commits behind head on main.

Files Patch % Lines
databricks/sdk/dbutils.py 87.75% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #618      +/-   ##
==========================================
- Coverage   57.89%   57.74%   -0.16%     
==========================================
  Files          44       47       +3     
  Lines       27956    32418    +4462     
==========================================
+ Hits        16185    18719    +2534     
- Misses      11771    13699    +1928     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@mgyucht mgyucht left a comment

Choose a reason for hiding this comment

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

Few nits but otherwise looks good.

databricks/sdk/dbutils.py Outdated Show resolved Hide resolved
databricks/sdk/dbutils.py Outdated Show resolved Hide resolved
databricks/sdk/dbutils.py Outdated Show resolved Hide resolved
@@ -241,6 +243,75 @@ def __getattr__(self, util) -> '_ProxyUtil':
name=util)


@dataclass
class OverrideResult:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? We just return result.result every time. Might as well return the result itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to provide a type that wraps a None result from the override. Some of the functions can then safely return None, if the override is not found or is not able to run.

kartikgupta-db and others added 3 commits April 18, 2024 14:37
Co-authored-by: Miles Yucht <miles@databricks.com>
Signed-off-by: Kartik Gupta <88345179+kartikgupta-db@users.noreply.github.com>
Co-authored-by: Miles Yucht <miles@databricks.com>
Signed-off-by: Kartik Gupta <88345179+kartikgupta-db@users.noreply.github.com>
Signed-off-by: Kartik Gupta <88345179+kartikgupta-db@users.noreply.github.com>
Copy link

This PR breaks backwards compatibility for databrickslabs/blueprint downstream. See build logs for more details.

Running from downstreams #81

@mgyucht mgyucht enabled auto-merge April 30, 2024 15:59
@mgyucht mgyucht added this pull request to the merge queue May 1, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants