use ssh-tunnel port if present #38
Conversation
|
Hi @ssendev. Thanks for this. A couple of things:
Thanks. |
ssendev
force-pushed
the
patch-1
branch
2 times, most recently
from
07e893e to
b5785bd
Compare
|
Overall, looks good to me. I don't use ssh-tunnel, so it would be good to get a review from someone who does. |
| @@ -42,6 +48,7 @@ module.exports = { | |||
| }, | |||
| redisDeployClient: function(context) { | |||
| var redisOptions = this.pluginConfig; | |||
| redisOptions.port = this.readConfig('port'); | |||
There was a problem hiding this comment.
@lukemelia How do you feel about this modifying the config object? Feels like this is a perfect candidate for need to have passed in a clone of the config so that any modifications to it don't affect anything outside this hook.
There was a problem hiding this comment.
@achambers Every default config value has this issue. Maybe the right path is for ember-cli-deploy to clone the config once per plugin. Anyway, not an issue for this PR in particular.
|
@duizendnegen Was it you who uses ssh-tunnel? |
|
No, don't even use Redis - not sure who does On Tue, Nov 10, 2015 at 7:40 PM, Aaron Chambers notifications@github.com
|
|
@achambers it's me :) and I think this could PR could work well indeed. I'm a bit weary of using defaults that explicitly refer to other plugins but we do it already for a few other things anyway and this will probably lower the learning curve also for the ssh tunnel plugin so in general I'm 👍 |
|
@lukemelia I think we can merge this if you agree |
|
@ssendev sorry for the delay in merging & thanks for the PR |
use ssh-tunnel port if present
this allows to use the dynamic port from ssh-tunnel instead of hard coding one.