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: do not remove mounted files directory, fixes #6188 #6199

Merged
merged 2 commits into from
May 20, 2024

Conversation

rfay
Copy link
Member

@rfay rfay commented May 16, 2024

The Issue

It turns out that with mutagen enabled, all of our import-files and ddev pull actions will fail because the host-side target files directory gets removed during the import process.

When mutagen is enabled, then each of the upload_dirs are bind-mounted from the host. In this situation, the existing code removes the host-side directory, breaking the bind-mount because the host source dir has been removed.

ddev restart fixes everything, but that's just a workaround.

How This PR Solves The Issue

  • Purge the source directory instead of removing it
  • Use otiai10/copy to do the copying instead of fileutil.CopyDir (which requires the target directory not to exist)

Manual Testing Instructions

With a Drupal (and all other CMS) directory, and with mutagen enabled, ddev import-files --src=<tarball> and verify that the files get loaded into the target directory

Same with ddev pull <anything>

Automated Testing Overview

  • I improved TestDdevImportFiles to verify with import-files that this succeeds. Our tests should have caught this before, especially in the import-files testing. It makes sense that we didn't catch it in the TestPantheonPull test because we don't use mutagen there.

Related Issue Link(s)

Release/Deployment Notes

Copy link

github-actions bot commented May 16, 2024

@rfay rfay force-pushed the 20240516_pantheon_pull_mutagen branch from 1c22d43 to 40e5161 Compare May 20, 2024 15:32
Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

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

This looks great to me!


Not related to this PR, but:

Use otiai10/copy to do the copying instead of fileutil.CopyDir (which requires the target directory not to exist)

I'm not sure, but maybe we can use a wrapper for otiai10/copy in fileutil because it's not always obvious in the code why it's used, and if we wrap it, we don't need to do an import for that library in many places.

@rfay
Copy link
Member Author

rfay commented May 20, 2024

I'm not sure, but maybe we can use a wrapper for otiai10/copy in fileutil because it's not always obvious in the code why it's used, and if we wrap it, we don't need to do an import for that library in many places.

otiai10/copy is so much better than the ancient home-grown one we use in general. I just haven't been willing to disrupt so much code by replacing the old one. It would be reasonable to do that with a wrapper.

@stasadev
Copy link
Member

Yes, I meant not to replace fileutil.CopyDir, but to add some new function so that it can be used in the same way.

@rfay
Copy link
Member Author

rfay commented May 20, 2024

I think otaia10/copy may even have an option in its options for that, so we could do that cleanly. It think it might be better to just disrupt everything and get rid of the current behavior (requiring deleted directory)

@rfay rfay merged commit 17abdad into ddev:master May 20, 2024
20 checks passed
@rfay rfay deleted the 20240516_pantheon_pull_mutagen branch May 20, 2024 23:19
jonesrussell pushed a commit to jonesrussell/ddev that referenced this pull request Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants