Adapt --ssh flag to work with Docker and Docker Compose. #4240
Conversation
|
After #4241 is addressed/merged, I can rebase so that tests pass. |
- Introduce a new `<scheme>:` prefix to the connection string provided to `--ssh`. Current accepted values are `docker:`, `docker-compose:`, and `ssh:`. This prefix can be omitted and a default of `ssh:` will be assumed. - Additionally make the `<user>@` option an explicit value separate from `<host>` so that it can be passed via CLI flag to Docker processes. - Refactor the `Runner#run_ssh_command` method to respond to different schemes. - Update existing functional test to accept escaped output. (Debug output of the unescaped command was removed during refactor to simplify the implementation.) - Add unit tests for updated `parse_ssh_url`. - Add functional test for `docker:` prefix.
The help text for the `--ssh` flag is now an unwrappable 61 characters long, leads to a failed functional test in `help.feature`. Strip out the `--ssh` global flag before passing to `wc -L`.
d9603c9 to
f3de62a
Compare
Add functional test to support.
| if ( $path ) { | ||
| $pre_cmd .= "cd {$path}; "; | ||
| if ( ! empty( $bits['path'] ) ) { | ||
| $pre_cmd .= 'cd ' . escapeshellarg( $bits['path'] ) . '; '; |
There was a problem hiding this comment.
I'm wondering if it would be better to set/override the --path arg below.
There was a problem hiding this comment.
Why do you think it would be better?
There was a problem hiding this comment.
I wonder so many things; I should probably try to cut down. General uninformed feeling:
- Less code, takes advantage of built-in escaping below.
- Doing it "the WP-CLI way."
- Currently, if user supplies path in connection string AND
--patharg, the--patharg "wins" even though it's not clear why or how.
After writing out point 3, though, I realize changing it would be rather arbitrary. If we wanted to be explicit, we could just drop support for path in the connection string and direct users to the --path arg.
There was a problem hiding this comment.
Thanks for clarifying, @chriszarate.
At this point, my preference would be to keep the behavior as it exists. The original decision was intentional: mimic the behavior of ssh'ing to a specific directory, and then running the wp executable. This lets you use a project-specific wp-cli.yml if you have multiple projects on the server.
|
@chriszarate This looks pretty good to me. Thanks for taking the time to write out the unit tests too. Is there any remaining work you'd like to perform on the PR before I merge? |
|
@danielbachhuber No, no remaining work from me. Thanks! |
|
👌 Thanks for your work on this! |
|
This change did not considered the |
Introduce a new
<scheme>:prefix to the connection string provided to--ssh. Current accepted values aredocker:,docker-compose:, andssh:. This prefix can be omitted and a default ofssh:will be assumed.Additionally make the
<user>@option an explicit value separate from<host>so that it can be passed via CLI flag to Docker processes.Refactor the
Runner#run_ssh_commandmethod to respond to different schemes.Update existing functional test to accept escaped output. (Debug output of the unescaped command was removed during refactor to simplify the implementation.)
Add unit tests for updated
parse_ssh_url.Add functional test for
docker:prefix.Remove
--sshglobal flag from 40-column wrap test. The help text for the--sshflag is now an unwrappable 61 characters long, leads to a failed functional test inhelp.feature. Strip out the--sshglobal flag before passing towc -L. Not sure what the best solution is here; this seemed like the most straightforward to me.