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 disabling redis #785

Open
wants to merge 3 commits into
base: htmx
Choose a base branch
from
Open

Conversation

elfjes
Copy link
Collaborator

@elfjes elfjes commented Apr 23, 2024

Add a boolean environment variable "ARGUS_DISABLE_REDIS" that, when set, will tell channels to use an InMemoryChannel instead of Redis so that we may run the backend without having a redis server running

Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

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

  1. This needs to be rebased, I think the upstream branch has seen a force-push since this was created, and now this PR seems to include @hmpf's old commits.
  2. While disabling Redis might be fine in a development environment, this setting needs to come with a big fat warning that it is not suitable for production use. In-memory channels are single-process only, and that's not how you would deploy the Argus in any production setting.

@hmpf hmpf self-requested a review April 25, 2024 10:35
@hmpf
Copy link
Contributor

hmpf commented Apr 25, 2024

  1. While disabling Redis might be fine in a development environment, this setting needs to come with a big fat warning that it is not suitable for production use. In-memory channels are single-process only, and that's not how you would deploy the Argus in any production setting.

What @lunkwill42 is probably trying to say is that you need to document the setting where the other settings are in the docs, and maybe only turn it on if DEBUG is also on. after rebasing :) I should really write that "how to merge"-doc...

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.39%. Comparing base (d8e30c8) to head (ab91dc4).
Report is 5 commits behind head on htmx.

Additional details and impacted files
@@            Coverage Diff             @@
##             htmx     #785      +/-   ##
==========================================
- Coverage   84.62%   84.39%   -0.23%     
==========================================
  Files          75       78       +3     
  Lines        3752     3787      +35     
==========================================
+ Hits         3175     3196      +21     
- Misses        577      591      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

sonarcloud bot commented Apr 25, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@elfjes
Copy link
Collaborator Author

elfjes commented Apr 25, 2024

I've limited the flag only when debug is also set, but this doesn't work if debug is managed statically in a different file. Consider the following:

For security reasons, DEBUG is set statically to False in settings.prod. However, this is done only after settings.base has been imported. This means that in theory DEBUG and ARGUS_DISABLE_REDIS may be set as environment variables, which would trigger disabling redis. Then DEBUG would be reset to False in settings.prod, but redis would still be disabled. Sounds like a (minor) issue, but I'm not quite sure how to deal with that (ie only evaluate ARGUS_DISABLE_REDIS after all other settings have been loaded...

Copy link

Test results

       7 files     574 suites   21m 33s ⏱️
   462 tests    461 ✔️ 1 💤 0
3 234 runs  3 227 ✔️ 7 💤 0

Results for commit 8809dcc.

@lunkwill42
Copy link
Member

If I'm reading #630 correctly, the mechanism it introduces will also use Redis for conveying messages about notifications to the new qcluster command. We'd have to remember to also include a warning in #630 that disabling Redis would disable notifications and possibly all other background processing functionality we might add using django-q2 later. This could also be ok in a development environment, but it could also be very bad, depending on what you're developing.

Copy link
Contributor

@hmpf hmpf left a comment

Choose a reason for hiding this comment

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

I think moving this to the dev settings file maybe makes the most sense.

Hmm...

@hmpf hmpf added the after-demo Need not work for demo label Jul 23, 2024
@hmpf hmpf removed after-demo Need not work for demo labels Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Changes requested
Development

Successfully merging this pull request may close these issues.

4 participants