- 
                Notifications
    You must be signed in to change notification settings 
- Fork 77
Add method to upload DuckDB files to Unity Catalog Volume with tests #2024
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
base: feature/add_local_dashboards
Are you sure you want to change the base?
Add method to upload DuckDB files to Unity Catalog Volume with tests #2024
Conversation
| ❌ 25/27 passed, 2 failed, 1m18s total ❌ test_transpile_sql_file: AssertionError (8.189s)❌ test_transpile_sql_file: AssertionError (10.968s)Running from acceptance #2738 | 
|  | ||
| # Validate inputs | ||
| if not os.path.exists(local_file_path): | ||
| print(f"Error: Local file not found: {local_file_path}") | 
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.
We'll want to use logger.error() statements here as opposed to printing to the console. Here is a good reference: https://github.com/databrickslabs/lakebridge/blob/main/src/databricks/labs/lakebridge/assessments/configure_assessment.py#L43
        
          
                src/databricks/labs/lakebridge/assessments/dashboards/dashboard_manager.py
          
            Show resolved
            Hide resolved
        
      | url = f"{workspace_url}/api/2.0/fs/files{volume_path}" | ||
|  | ||
| with open(local_file_path, 'rb') as f: | ||
| response = requests.put(url, headers=headers, data=f) | 
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.
@radhikaathalye-db could you pull the upstream changes from the branch feature/add_local_dashboards. We'll want to use the Python SDK (WorkspaceClient) to upload files to the UC volume vs. using the REST API. Thanks!
        
          
                src/databricks/labs/lakebridge/assessments/dashboards/dashboard_manager.py
          
            Show resolved
            Hide resolved
        
      | @patch("os.path.exists") | ||
| @patch("builtins.open", new_callable=MagicMock) | ||
| def test_upload_duckdb_to_uc_volume_failure(mock_open, mock_exists, dashboard_manager): | ||
| mock_exists.return_value = True | 
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.
Wouldn't overwrite = True avoid this type of scenario?
2dcdf50    to
    25c1724      
    Compare
  
    
Changes
What does this PR do?
Add a new method to the DashboardManager class to upload DuckDB extract to UC volume. Add tests for the same.
Relevant implementation details
Caveats/things to watch out for when reviewing:
Linked issues
Resolves #..
Functionality
databricks labs lakebridge ...Tests