-
-
Notifications
You must be signed in to change notification settings - Fork 580
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
refactor: revert upload_dirs=false
, fixes #5131
#5134
Conversation
Download the artifacts for this pull request:
See Testing a PR |
upload_dirs=false
, fixes #5131
util.Warning("You have Mutagen enabled and your '%s' project type doesn't have `upload_dirs` set.", app.Type) | ||
util.Warning("For faster startup and less disk usage, set upload_dirs to where your user-generated files are stored.") | ||
util.Warning("If this is intended you can disable this warning by running `ddev config --upload-dirs=false`.") | ||
// TODO: Implement --no-upload-dirs-warning in new PR | ||
util.Warning("If this is intended you can disable this warning with `ddev config --no-upload-dirs-warning`.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
util.Warning("If this is intended you can disable this warning with `ddev config --no-upload-dirs-warning`.") | |
//util.Warning("If this is intended you can disable this warning with `ddev config --no-upload-dirs-warning`.") |
does not make much sense to show this info even it doesn't work at the moment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to follow it immediately with the actual replacement, so it's ok.
The Issue
We tried to support a multi-type upload_dirs but it added a lot of complexity to the code.
Unfortunately, I've decided to revert that and go back to a simpler approach.
An additional PR will be required to add a new config that turns off warnings about upload_dirs
Also accidentally fixes
config.*.yaml
resets upload_dirs to empty #5131by going back to a simple array for the declaration.
How This PR Solves The Issue
Manual Testing Instructions
Many things have to be re-tested:
config.*.yaml
resets upload_dirs to empty #5131 - emptyconfig.*.yaml
was resulting in upload_dirs being emptiedddev import-files
should work any time upload_dirs are implied or specified #5127 - import-files needs to work on Laravel and Craft if import-dirs specifiedupload-dirs
is false #5126 - panic with upload-dirsupload_dir
should be able to be set to empty to prevent bind-mounting #4796 - Of course we need a way to do that.Automated Testing Overview
I think the tests cover the things that matter
Related Issue Link(s)
Release/Deployment Notes