search

consolidation / site-alias

Manage alias records for local and remote sites.
Other
60 stars 15 forks source link

Add support for aliases that use Docker Compose transport. #30

Closed weitzman closed 5 years ago

weitzman commented 5 years ago

Overview

This pull request:

Q A
Bug fix? no
New feature? yes
Has tests? no
BC breaks? no
Deprecations? no
greg-1-anderson commented 5 years ago

I have reservations about service, especially remapping it back to remote-host.

Before starting with code, could we design the alias schema to support both docker exec on the local machine, and also ssh <remote> docker exec on a remote machine? Once it's clear what the exec lines need to be, and what data we'd like to indicate those destinations, I'll feel better about proposed solutions.

weitzman commented 5 years ago

I have not seen the use case for ssh and then docker exec. The docker compose transport should support remote hosts when your DOCKER_HOST env variable is set accordingly.

Happy to discuss more regardless.

greg-1-anderson commented 5 years ago

Maybe we could offer a feature to set DOCKER_HOST based on information in the site alias. I don't know if this would be better off as a docker-specific feature, or if we should have a general-purpose environment-variable-setting feature.

I'd still like to see if we can provide docker support just in site-alias / site-process. It seems like this should be possible, although I have not tried to refactor the Drush PR into the other components.

greg-1-anderson commented 5 years ago

This is one step more general than before, which is good. I feel like overloading isRemote here is not the right thing. This library is still young; it wouldn't hurt at all if we needed to make breaking changes and bump to 3.x.

I still need to think a bit about how Drush is using isRemote and how it alters the flow of control. Ideally, we'd have some other method that encapsulated the idea of ssh and docker, and returned isRemote() || isDocker().

greg-1-anderson commented 5 years ago

The idea being, if we updated SiteAlias + SiteProcess with another transport, then we shouldn't have to change Drush, or worry about the side effects of overloading isRemote()