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

Sample - Fix exports from Workspace to Windows FS #448

Merged
merged 4 commits into from
May 5, 2022

Conversation

stormwindy
Copy link
Collaborator

No description provided.

@stormwindy stormwindy added the bug Broken functionality label Apr 2, 2022
@stormwindy stormwindy self-assigned this Apr 2, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 2, 2022

Codecov Report

Merging #448 (151ff18) into master (1e852b5) will increase coverage by 0.34%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #448      +/-   ##
==========================================
+ Coverage   84.50%   84.84%   +0.34%     
==========================================
  Files          42       42              
  Lines        3084     3135      +51     
==========================================
+ Hits         2606     2660      +54     
+ Misses        478      475       -3     
Impacted Files Coverage Δ
databricks_cli/workspace/api.py 87.50% <100.00%> (+0.34%) ⬆️
databricks_cli/pipelines/api.py 96.33% <0.00%> (ø)
databricks_cli/click_types.py 87.50% <0.00%> (+0.37%) ⬆️
databricks_cli/pipelines/cli.py 96.20% <0.00%> (+2.83%) ⬆️

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 1e852b5...151ff18. Read the comment docs.

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 added the windows bugs that occur on Windows OS label May 5, 2022
@nfx nfx merged commit c695b8d into databricks:master May 5, 2022
@stormwindy
Copy link
Collaborator Author

@nfx I actually created this as a sample solution and didn't spend much time. I couldn't get back to it since then to finalize. Are we certain about the way I handled it (espc. regex)?

@thaiphv
Copy link

thaiphv commented Jul 12, 2022

Was this merged intentionally?

@@ -166,15 +168,16 @@ def import_workspace_dir(self, source_path, target_path, overwrite, exclude_hidd
'continue.').format(cur_src, extensions))

def export_workspace_dir(self, source_path, target_path, overwrite, headers=None):
if os.path.isfile(target_path):
os_compatible_target_path = re.sub(LOCAL_OS_COMPATIBLE_PATH_REGEX, '_', target_path)
Copy link

Choose a reason for hiding this comment

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

This will replace C:\\Users\\admin with C_Users\\admin. Effectively it converts an absolute path to a relative path.

pietern added a commit to pietern/databricks-cli that referenced this pull request Jul 13, 2022
@pietern
Copy link
Contributor

pietern commented Jul 13, 2022

Ouch! Thanks for flagging, @thaiphv. I'm reverting the change in #516.

pietern added a commit that referenced this pull request Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Broken functionality windows bugs that occur on Windows OS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants