Skip to content
This repository has been archived by the owner on Jun 8, 2023. It is now read-only.

sepearte URL and PORT specification #1329

Closed
wants to merge 1 commit into from
Closed

sepearte URL and PORT specification #1329

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Feb 15, 2014

No description provided.

@technicalpickles
Copy link
Contributor

Why not include a port in the REDIS_URL?

@parkr
Copy link
Contributor

parkr commented Feb 15, 2014

@technicalpickles I think the wish is to be able to manipulate the port without having to know the URL.

@technicalpickles
Copy link
Contributor

Do you know the HOSTNAME? The URL would just be redis://HOSTNAME:PORT

@ghost
Copy link
Author

ghost commented Feb 16, 2014

@parkr @technicalpickles
the reason for this is when setting up the bot using heroku, and a third party redis provider it conked out because of a custom port number

@technicalpickles
Copy link
Contributor

@sableloki I'm still not sure I follow why including the port number in the REDIS_URL doesn't work. You can see info has the URI information, which includes info.port, so that means you should be able to specify a URL with the port and have it work.

a third party redis provider it conked out because of a custom port number

Could you elaborate on 'conked out'? Were there any particular errors or log messages?

@ghost
Copy link
Author

ghost commented Feb 17, 2014

it would attempt to connect to the default port and then append :12670 (the custom port)

@technicalpickles
Copy link
Contributor

I mispoke earlier, the environment variables it recognizes are:

  • REDISTOGO_URL
  • REDISCLOUD_URL
  • BOXEN_REDIS_URL

Could you make sure one of those is set with the port in the url, ie redis://somehost:12670?

It's also worth noting that this would break redis-brain for people who expect that behavior to work, of being able to include the port in the URL.

@ghost
Copy link
Author

ghost commented Feb 19, 2014

i had REDISCLOUD_URL set to exactly that

@technicalpickles
Copy link
Contributor

If that's the case, seems like a bug in the implementation. I think I rather see that fixed than adding another environment variable.

In general, I find it preferable to have *_URL variables, rather than several *_HOST, *_PORT, *_USER, *_PASS variables. To start, it's less code, but it's also simpler to understand how to affect change in configuration.

@ghost
Copy link
Author

ghost commented Feb 22, 2014

thats a fair argument i however prefer to have more verbose config as it makes debugging that much easier as you can drill right down the point.

@technicalpickles
Copy link
Contributor

If we need debugging, should just add robot.logger.debug calls with connection information then.

@@ -18,7 +19,7 @@ Redis = require "redis"

module.exports = (robot) ->
info = Url.parse process.env.REDISTOGO_URL or process.env.REDISCLOUD_URL or process.env.BOXEN_REDIS_URL or 'redis://localhost:6379'
client = Redis.createClient(info.port, info.hostname)
client = Redis.createClient(process.ENV.REDIS_PORT, info.hostname)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

process.ENV is not valid. process.env is what you're looking for.

@therealklanni
Copy link
Contributor

I agree with @technicalpickles on this. The port can be set via one of the URL env vars that already exists. Adding another env var would add undue complexity. I vote this issue be closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants