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

fixes filesystem dest on windows #1335

Merged
merged 5 commits into from
May 8, 2024

Conversation

rudolfix
Copy link
Collaborator

@rudolfix rudolfix commented May 7, 2024

Description

filesystem destination was not working well with local windows path. we have more and more windows users so this needs ot be corrected.

  1. uses native local paths and native pathlib
  2. normalizes all paths to pathlib separator
  3. adds end to end filesystem tests on windows and mac + local filesystems
  4. tbd. handling of windows extended paths \?\ (to support more than 256 chars in path)

Copy link

netlify bot commented May 7, 2024

Deploy Preview for dlt-hub-docs canceled.

Name Link
🔨 Latest commit aca319b
🔍 Latest deploy log https://app.netlify.com/sites/dlt-hub-docs/deploys/663b533d9fe2d30008def2aa

@rudolfix rudolfix marked this pull request as ready for review May 7, 2024 21:21
@rudolfix rudolfix requested a review from sh-rp May 7, 2024 21:21
@rudolfix rudolfix force-pushed the rfix/fixes-filesystem-dest-on-windows branch from fc718d5 to 9f99445 Compare May 8, 2024 07:02
@sh-rp
Copy link
Collaborator

sh-rp commented May 8, 2024

LGTM, the new test is failing on mac and ubuntu now, works for windows though :D

@sh-rp
Copy link
Collaborator

sh-rp commented May 8, 2024

One note, not directly related: maybe we should organize the tests slightly differently. In the test-common workflow we now are listing tests and folders behind pytest commands. This is prone to us adding tests that might not be run on CI in the end or might be run on the wrong platform. Maybe we should reorganize them just a bit at some point to remove complexity.

sh-rp
sh-rp previously approved these changes May 8, 2024
@rudolfix
Copy link
Collaborator Author

rudolfix commented May 8, 2024

One note, not directly related: maybe we should organize the tests slightly differently. In the test-common workflow we now are listing tests and folders behind pytest commands. This is prone to us adding tests that might not be run on CI in the end or might be run on the wrong platform. Maybe we should reorganize them just a bit at some point to remove complexity.

right! this structure does the following:

  1. starts with minimum deps and runs tests that should run like that (no additional dependencies)
  2. adds extras runs more tests
  3. runs all pipeline tests (more extras) that do not require credentials

we could reorganize the folders or mark the tests. that still may cause problems so maybe we should use marks to exclude tests (not marked tests are run everywhere)

@rudolfix rudolfix force-pushed the rfix/fixes-filesystem-dest-on-windows branch from 9f99445 to aca319b Compare May 8, 2024 10:26
@rudolfix rudolfix merged commit dfd4d89 into devel May 8, 2024
48 of 50 checks passed
@rudolfix rudolfix deleted the rfix/fixes-filesystem-dest-on-windows branch May 8, 2024 13:16
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.

2 participants