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

Adapt --ssh flag to work with Docker and Docker Compose. #4240

Merged
merged 3 commits into from Jul 24, 2017

Conversation

3 participants
@chriszarate
Contributor

chriszarate commented Jul 23, 2017

  • 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.

  • Remove --ssh global flag from 40-column wrap test. 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. Not sure what the best solution is here; this seemed like the most straightforward to me.

@chriszarate

This comment has been minimized.

Show comment
Hide comment
@chriszarate

chriszarate Jul 23, 2017

Contributor

After #4241 is addressed/merged, I can rebase so that tests pass.

Contributor

chriszarate commented Jul 23, 2017

After #4241 is addressed/merged, I can rebase so that tests pass.

chriszarate added some commits Jul 23, 2017

Adapt --ssh flag to work with Docker and Docker Compose.
- 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.
Remove --ssh global flag from 40-column wrap test.
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`.
if ( $path ) {
$pre_cmd .= "cd {$path}; ";
if ( ! empty( $bits['path'] ) ) {
$pre_cmd .= 'cd ' . escapeshellarg( $bits['path'] ) . '; ';

This comment has been minimized.

@chriszarate

chriszarate Jul 24, 2017

Contributor

I'm wondering if it would be better to set/override the --path arg below.

@chriszarate

chriszarate Jul 24, 2017

Contributor

I'm wondering if it would be better to set/override the --path arg below.

This comment has been minimized.

@danielbachhuber

danielbachhuber Jul 24, 2017

Member

Why do you think it would be better?

@danielbachhuber

danielbachhuber Jul 24, 2017

Member

Why do you think it would be better?

This comment has been minimized.

@chriszarate

chriszarate Jul 24, 2017

Contributor

I wonder so many things; I should probably try to cut down. General uninformed feeling:

  1. Less code, takes advantage of built-in escaping below.
  2. Doing it "the WP-CLI way."
  3. Currently, if user supplies path in connection string AND --path arg, the --path arg "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.

@chriszarate

chriszarate Jul 24, 2017

Contributor

I wonder so many things; I should probably try to cut down. General uninformed feeling:

  1. Less code, takes advantage of built-in escaping below.
  2. Doing it "the WP-CLI way."
  3. Currently, if user supplies path in connection string AND --path arg, the --path arg "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.

This comment has been minimized.

@danielbachhuber

danielbachhuber Jul 24, 2017

Member

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.

@danielbachhuber

danielbachhuber Jul 24, 2017

Member

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.

This comment has been minimized.

@chriszarate

chriszarate Jul 24, 2017

Contributor

Makes sense.

@chriszarate

chriszarate Jul 24, 2017

Contributor

Makes sense.

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Jul 24, 2017

Member

@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?

Member

danielbachhuber commented Jul 24, 2017

@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?

@chriszarate

This comment has been minimized.

Show comment
Hide comment
@chriszarate

chriszarate Jul 24, 2017

Contributor

@danielbachhuber No, no remaining work from me. Thanks!

Contributor

chriszarate commented Jul 24, 2017

@danielbachhuber No, no remaining work from me. Thanks!

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Jul 24, 2017

Member

👌 Thanks for your work on this!

Member

danielbachhuber commented Jul 24, 2017

👌 Thanks for your work on this!

@danielbachhuber danielbachhuber merged commit b384b1c into wp-cli:master Jul 24, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@danielbachhuber danielbachhuber added this to the 1.3.0 milestone Jul 24, 2017

@drzraf

This comment has been minimized.

Show comment
Hide comment
@drzraf

drzraf Jun 11, 2018

This change did not considered the -i option of docker exec.
Without this, stdin can not be passed which keeps wp db import from running.

drzraf commented Jun 11, 2018

This change did not considered the -i option of docker exec.
Without this, stdin can not be passed which keeps wp db import from running.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment