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

fix: Windows fails with a path issue #89

Merged
merged 2 commits into from
Jul 13, 2023

Conversation

visch
Copy link
Member

@visch visch commented Jul 13, 2023

Fixes #87

@visch visch changed the title Windows Tests should fail if we're doing an E2E test now fix: Windows Tests should fail if we're doing an E2E test now Jul 13, 2023
@visch visch changed the title fix: Windows Tests should fail if we're doing an E2E test now fix: Windows fails with a path issue Jul 13, 2023
@pnadolny13
Copy link
Contributor

@visch this is great! It doesnt look like it ran the CI tests since the PR is from a fork but is https://github.com/visch/target-snowflake/actions/runs/5545409577/jobs/10124299586 saying that they all passed? I'm guessing you had to configure your own credentials?

sqlalchemy.text stripped a slash, which caused windows to fail so we used bound parameters instead

This is a good catch. It might be a good idea to use those everywhere but that doesnt need to happen in this PR. I can open a separate issue for that.

@visch
Copy link
Member Author

visch commented Jul 13, 2023

@visch this is great! It doesnt look like it ran the CI tests since the PR is from a fork but is https://github.com/visch/target-snowflake/actions/runs/5545409577/jobs/10124299586 saying that they all passed? I'm guessing you had to configure your own credentials?

sqlalchemy.text stripped a slash, which caused windows to fail so we used bound parameters instead

This is a good catch. It might be a good idea to use those everywhere but that doesnt need to happen in this PR. I can open a separate issue for that.

Yeah I configured my own creds!

This is a good catch. It might be a good idea to use those everywhere but that doesnt need to happen in this PR. I can open a separate issue for that.

Makes sense to me :D

@pnadolny13 pnadolny13 merged commit 97cb0c8 into MeltanoLabs:main Jul 13, 2023
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.

File doesn't exist when putting batch to internal stage using Windows machine
2 participants