Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Socket should also connect to redis specified in .env #213

Merged
merged 3 commits into from
May 4, 2016

Conversation

knvpk
Copy link
Contributor

@knvpk knvpk commented May 4, 2016

In raising this pull request, I confirm the following (please check boxes):

  • [*] I have read and understood the contributing guidelines?
  • [*] I have checked that another pull request for this purpose does not exist.
  • [*] I have considered, and confirmed that this submission will be valuable to others.
  • [*] Do the PHPCI tests pass?
  • [*] Does the StyleCI test pass?

NOTE: The last 2 are not required to open a PR and can be done afterwards /
while the PR is open.


Description of change

Please provide a description of the change here.

Im actually using the redis in docker means another machine it has its own ip address, all are working fine but notifications when deploying are not working, when i debugged in the socket.js it is connecting to local redis instance rather than redis that is specified in the .env file. I think it is better to connect to redis which is specified in .env. Generally when using the remote redis it is preferrable to have a password so i have given the option to specify the redis_password

Im actually using the redis in docker means another machine it has its own ip address, all are working fine but notifications when deploying are not working, when i debugged in the `socket.js` it is connecting to local redis instance rather than redis that is specified in the .env file. I think it is better to connect to redis which is specified in .env. Generally when using the remote redis it is preferrable to have a password so i have given the option to specify the `redis_password`
@knvpk knvpk changed the title **[FIX]** Socket should also connect to redis specified in .env [FIX] Socket should also connect to redis specified in .env May 4, 2016
@REBELinBLUE
Copy link
Owner

Thanks, I'll merge it this evening

@REBELinBLUE REBELinBLUE added the Bug label May 4, 2016
@REBELinBLUE
Copy link
Owner

Could you add the default values to .env.example as well?

Thanks

db: process.env.REDIS_DATBASE || 0
port: process.env.REDIS_PORT || 6379,
host: process.env.REDIS_HOST || '127.0.0.1',
db: process.env.REDIS_DATBASE || 0,
Copy link
Owner

Choose a reason for hiding this comment

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

Just noticed, this should be REDIS_DATABASE (I know, it was wrong before also). Would you mind changing it?

Thanks

knvpk added 2 commits May 4, 2016 19:27
DATBASE  -> DATABASE
Same as in socketjs
@knvpk
Copy link
Contributor Author

knvpk commented May 4, 2016

Default values is nothing for password and database, so im leaving it empty.

@REBELinBLUE REBELinBLUE merged commit 84fc8af into REBELinBLUE:master May 4, 2016
@REBELinBLUE
Copy link
Owner

Thanks

@REBELinBLUE
Copy link
Owner

Turns out that doesn't work, leaving them empty results in them becoming empty values in the config, not the defaults so I have set the defaults in .env.example

@REBELinBLUE REBELinBLUE changed the title [FIX] Socket should also connect to redis specified in .env Socket should also connect to redis specified in .env May 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants