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

Allow for Redis environment variables #141

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alanakirby
Copy link

This allows for you to run your storage anywhere, as the host name is not often just redis. This change will make no impact on exisiting users as it allows for the env var to be set or the defaults as set before.

This allows for you to run your storage anywhere, as the host name is not often just redis. This change will make no impact on exisiting users as it allows for the env var to be set or the defaults as set before.
Copy link
Contributor

@azh-r azh-r left a comment

Choose a reason for hiding this comment

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

Hi @alanakirby,

This will be useful flexibility to have - thanks for pointing this out! Please could you look at a comment I've left to show a way in which we can use the existing code for grabbing from environment variables to help us do this in the same way as other settings variables?

Comment on lines +101 to +103
REDIS_HOST=os.environ('REDIS_HOST', 'redis')
REDIS_PORT=os.environ('REDIS_PORT', '6379')
REDIS_DB=os.environ('REDIS_DB', '0')
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this use the existing code to configure settings by setting environment variables. To do that, you can add these REDIS_HOST, REDIS_PORT and REDIS_DB to the existing loop in

for envvar in ['SMTP_PORT', 'SMTP_USERNAME', 'SMTP_PASSWORD', 
...

And then to set the defaults after the loop, set the defaults if not already set.

if not REDIS_HOST:
    REDIS_HOST='redis'
if not REDIS_PORT:
   ...

The settings module is clumsy at the moment at setting defaults, but sticking to the convention of having environment variables formatted like CANARY_REDIS_HOST and CANARY_REDIS_PORT using existing code, will help us when we refactor this module to make these types of changes easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @alanakirby,

Just checking you saw this response. We would like to get this merged in 🤘

Copy link
Author

Choose a reason for hiding this comment

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

Hi Jay! Its been so long since Ive worked on this project actually. Would your suggestion above allow for storage other than redis?

Copy link
Contributor

Choose a reason for hiding this comment

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

@alanakirby, I'm unsure this commit should attempt to do that. The reason I say this is because we use redis specific libraries through the codebase and so the redis python is unlikely to work with other storages. Which storages did you have in mind?

@azh-r azh-r added the question label May 19, 2023
@azh-r azh-r self-assigned this May 19, 2023
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.

3 participants