dokku / dokku-redis

a redis plugin for dokku
MIT License
256 stars 39 forks source link

redis:link does not complete correctly if REDIS_URL already set #73

Open crutch opened 7 years ago

crutch commented 7 years ago

While I was trying to setup our dokku powered application with redis, I set up manually REDIS_URL config variable for our app (I was creating staging env, replicating setup from our production machine and created REDIS_URL with correct staging redis DSN).

Then I attempted to create a dokku/docker link between redis and our app: dokku redis:link <redis> <app>

It finished by saying something like that REDIS_URL is already present in the config. However it was not presented in a way, which would clearly express that the whole command failed (i.e., the docker link was not created).

Since I was aware of the fact that I have entered a correct REDIS_URL manually, I did not consider the message as suspicious and did not check the link manually. Was very surprised to see connection failures in the app logs afterwards...

I suggest either a better explanation of redis:link result (the link was NOT created) or successful creation of link if already present REDIS_URL is correct.

josegonzalez commented 7 years ago

Do you know what the exact output was, as well as what the output of dokku config APP | grep REDIS was?

crutch commented 7 years ago
$ dokku config foo
=====> foo config vars
REDIS_URL: redis://bar:36f25a834f8303b3e82445e9649860fd7b1a81d20585079d4c46a38e25a74bb2@dokku-redis-bar:6379
$ dokku redis:link bar foo
Already linked as REDIS_URL
$ dokku redis:list
NAME      VERSION      STATUS   EXPOSED PORTS  LINKS
bar       redis:3.2.3  running  -              -
josegonzalez commented 7 years ago

Thats not the output i asked for. Can you run the exact dokku config command I ran?

crutch commented 7 years ago

@josegonzalez as you wish, exact dokku config command:

$ dokku config APP | grep REDIS
App name must begin with lowercase alphanumeric character

perhaps you would allow me to to change APP for an actual name of my application

dokku config foo | grep REDIS
REDIS_URL: redis://bar:36f25a834f8303b3e82445e9649860fd7b1a81d20585079d4c46a38e25a74bb2@dokku-redis-bar:6379

Sorry, what was the point of your comment? I do not find that grep that useful in this particular scenario.

josegonzalez commented 7 years ago

ha yes alright. I was hoping that adding the link would have created something like:

DOKKU_BLUE_REDIS_URL: dsn

in the output of my command. If you link a service and already have a value there, then it'll use a different url by default, similar to what happens on heroku.

crutch commented 7 years ago

Well, that did not happen. I created an empty app foo and empty redis service bar for this to try it out. All the outputs I posted are unfiltered.

josegonzalez commented 7 years ago

Can you run the following:

dokku --trace redis:link bar foo

and gist the output?

crutch commented 7 years ago

Sure, here it is.

josegonzalez commented 7 years ago

Ah I see. So you created the service, manually set the config variable, and then tried to link. The link fails because we detect that there is an env var that has the service url already set, so we don't actually write anything else on our end.

What you'd like is for us to fail because it's already set but we don't have the proper setup on our end or ensure all the link stuff is set on the service's side (plus the extra docker options).

I think the way forward here is to just ensure the docker options/links are set properly if we detect the link is there.

For now, I would unset the variable and let the plugin handle the linking. I'll leave this issue open until I/someone else gets around to implementing this functionality.

crutch commented 7 years ago

@josegonzalez yes, once I realized what was going on, I unset the config var and repeated the linking.

I see that you got my point - if the linking fails, it should fail more loudly. Or it should ensure that the docker link was created as well, apart from the config variable.

josegonzalez commented 7 years ago

Yeah. I'm marking this as an enhancement as dokku-redis is working as advertised - we say use redis:link in our docs - but it can definitely "clean up" after bad setup.