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 for Github clone on Windows #15143

Merged
merged 7 commits into from
Sep 3, 2024
Merged

Conversation

WWM-jschuba
Copy link
Contributor

@WWM-jschuba WWM-jschuba commented Aug 29, 2024

The GitHubRepository.get_directory() method did not work on Windows, because the slashes in the temp path were being removed by shlex. The fix is to wrap the path with "quotes". This is the proposed fix in the issue 13180 discussion.

closes #13180

Checklist

  • This pull request references any related issue by including "closes <link to issue>"

need to wrap Windows paths with "quotes"
@github-actions github-actions bot added arch:windows Related to the Windows OS bug Something isn't working labels Aug 29, 2024
Copy link
Collaborator

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

thank you for the PR! looks like we need pre-commit run --all-files here to pass CI

@WWM-jschuba
Copy link
Contributor Author

I read and ran the existing tests here. The core functionality of the Github clone command is untested. The tests all mock this away.

The functionality of GithubRepository.get_directory() is

  1. Receive references for (or create) two temp directories (content_source, content_destination) -> mocked by tests
  2. Clone the target Github Repo into content_source -> Silently fails because of test mocks.
  3. Copy contents of content_source into content_destination -> works, and is tested

Because the content_source temp directory is already created in the test mocks, with files inside, the clone command in step 2 fails silently (you cannot git clone into a non-empty directory). This may have been the intention of the test writer, so the tests would not download the large prefect.git repo.

However, only steps 1 and 3 are actually tested now, and the core purpose of the function, cloning a repo, is untested.

That is why the tests pass on Windows, but the module doesn't work in practice. Wrapping the path in quotes very straightforwardly fixes the issue for all OS's.

Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

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

LGTM

@zzstoatzz zzstoatzz removed the arch:windows Related to the Windows OS label Sep 3, 2024
@zzstoatzz zzstoatzz merged commit 6dc401d into PrefectHQ:main Sep 3, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows - Prefect GitHub get_directory does not clone any files to the local_path
3 participants