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 ddev exec message offered to wp users on import #1014

Merged
merged 4 commits into from
Jul 27, 2018

Conversation

rfay
Copy link
Member

@rfay rfay commented Jul 25, 2018

The Problem/Issue/Bug:

A user in slack pointed out that our prompted suggestion of wp search-replace results in an error because we quote it.

The prompt says

Wordpress sites require a search/replace of the database when the URL is changed. You can run "ddev exec 'wp search-replace [http://www.myproductionsite.example] http://wpsite.ddev.local'" to update the URLs across your database. For more information, see http://wp-cli.org/commands/search-replace/

The result if you do that is:

ddev exec 'Failed to execute command [wp search-replace [http://www.myproductionsite.exampleto update the URLs across your da] http://wpsite.ddev.local]: Failed to run docker-compose [-f /Users/brentr obbins/wpsite/.ddev/docker-compose.yaml exec -T web wp search-repl ace [http://www.myproductionsite.example] http://wpsite.ddev.local], err='exit status 126', stdout='OCI runtime exec failed: exec failed: container_linux.go :348: starting container process caused "exec: \"wp search-replace [http://www.my productionsite.example] http://wpsite.ddev.local\": stat wp search-replace [http://www.myproductionsite.example] http://wpsite.ddev.local: no such file or directory": unknown ', stderr=''

Because ddev exec is trying to run an executable called "wp search-replace ..." instead of just running wp.

How this PR Solves The Problem:

Remove the quotes around the command.

Manual Testing Instructions:

Automated Testing Overview:

Related Issue Link(s):

Release/Deployment notes:

@rfay rfay added this to the v1.1.0 milestone Jul 25, 2018
@rfay rfay self-assigned this Jul 25, 2018
@rfay rfay requested a review from andrewfrench July 25, 2018 23:48
@rickmanelius rickmanelius self-requested a review July 26, 2018 20:36
Copy link
Contributor

@rickmanelius rickmanelius left a comment

Choose a reason for hiding this comment

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

Confirmed this worked. We could equally update the default config.yml to reflect the *.ddev.local naming convention.

#hooks:

Un-comment and enter the production url and local url

to replace in your database after import.

#post-import-db:

- exec: "wp search-replace "

Even without that recommendation, this is a +1 to pull.

@rfay rfay force-pushed the 20180725_fix_wp_search_replace branch from 7461bf7 to f364903 Compare July 26, 2018 22:56
@andrewfrench
Copy link
Contributor

andrewfrench commented Jul 27, 2018

A project-wide search for exec: " shows this incorrect pattern being generated and suggested in many places:

grep -R --exclude-dir .gotmp --exclude-dir bin --exclude-dir vendor 'exec: "' .

Could these be updated as well?

Update: It seems wrapping the command in quotes in a hook does work, so updating the results of the grep and the WordPress post-import-db command might not be required, but it might as well be updated everywhere.

Copy link
Contributor

@andrewfrench andrewfrench left a comment

Choose a reason for hiding this comment

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

Small note about similar suggestions elsewhere in docs, but these changes are approved

@rfay
Copy link
Member Author

rfay commented Jul 27, 2018

Interestingly enough, the command to exec works inside quotes when used as a hook, and in some cases the quotes improve things, I can't give an example though. I think we're better to offer it without quotes where it doesn't need them.

rfay added 2 commits July 27, 2018 15:09
Although the hook argument can be done with or without quotes,
I agree it's less confusing to suggest doing it without
quotes. Because "ddev exec" works only without quotes in
most cases.
@rfay rfay merged commit 4466584 into ddev:master Jul 27, 2018
@rfay rfay deleted the 20180725_fix_wp_search_replace branch July 27, 2018 23: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.

None yet

3 participants