Added environment variables in aliases

consolidation / site-process

A thin wrapper around the Symfony Process Component that allows applications to use the Site Alias library to specify the target for a remote call.

Other

50 stars 19 forks source link

Added environment variables in aliases #47

Closed geek-merlin closed 5 years ago

geek-merlin commented 5 years ago

https://github.com/drush-ops/drush/issues/4170

Overview

This pull request:

Q	A
Bug fix?	yes/no
New feature?	yes/no
Has tests?	yes/no
BC breaks?	no
Deprecations?	yes/no

Summary

Short overview of what changed.

Description

Any additional information.

greg-1-anderson commented 5 years ago

This is a good start. Looks like it should work in the ssh transport, but might not work in the other transports. It will definitely be necessary to delegate to the transport before merging here. Unit tests for each transport would be good too.

geek-merlin commented 5 years ago

Thanks greg for your feedback. I can definitely move the method (and a part of me likes any abstraction and pluginification ;-). Alas, i do not completely understand why we should vary by transport. Let's think aloud:

What we do is e.g. wrap drush status so it becomes env PATH=foo drush status. A valid command that runs on any target. This might vary on windows but i don't see how this depends on the transport.

Researched the 4 transports (local ssh docker-compose vagrant) and only in docker i found a different possibility. But given that wrapping in env just works why should we add the complexity? Is there something i do not see?

greg-1-anderson commented 5 years ago

Test it with docker and see if it works. You might be right.

greg-1-anderson commented 5 years ago

It really seems better to me to use -e with docker, but I'm not sure if that is necessary.

geek-merlin commented 5 years ago

You simply can do it both ways, with marginal difference. As i understand it:

docker-compose -e foo=bar zong uses docker to set env vars foo=bar and execute zong process
docker-compose env foo=bar zong executes env foo=bar zong which in turn sets env var foo=bar and executes the zong process

What about if we move on and prepare what else is needed. As i understand it:

Document the env-vars option in alias example
Cover the new codepath in SiteProcessTest

I think i'll work on this in the next days.

If moshe or you or me or someone else comes up with a need to move this to the transport, it's fairly trivial and can be done anytime in this PR or a followup.

greg-1-anderson commented 5 years ago

Yeah, flexibility is the reason I wanted to see it in the transport. However, I'm not sure that there would ever be an environment where you couldn't run env to set environment variables, so it's probably fine to just move forward under that assumption.

geek-merlin commented 5 years ago

OK. We got

manually tested code
a passing test
a companion PR for docs: https://github.com/drush-ops/drush/pull/4173

Feedback appreciated!

greg-1-anderson commented 5 years ago

Looks good - thanks.

geek-merlin commented 5 years ago

Wooot! Thanks a lot for the good conversation and prompt responses!