Relax Redis requirement #739
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Connecting with the rediss: protocol won't work if TLS isn't enabled, and the error message is a generic "connection timed out".
nextstrain-bot
temporarily deployed
to
nextstrain-s-trs-relax--88ypky
tsibley
force-pushed
the
trs/relax-redis-requirement
branch
from
4ab3cf0
to
3945b21
Compare
… is not available
Rather than non-durably in process memory. This solves two issues:
1. It allows for production use without Redis as long as certain other
requirements are met. I'll be documenting these and relaxing the
Redis requirement in a subsequent commit. This is motivated by
CDC's deployment.¹
2. It makes development more like production, which is important as the
difference wasn't obvious and led to strange behaviour, e.g. across
server restarts.²
I knew the transient in-memory store was not ideal when I wrote it, but
it was very expedient! and I didn't want to address it further at the
time. This now rectifies that.
The configuration of the keyv library with a key namespace ensures that
no key name migration is needed with this change.
¹ <nextstrain/private#94>
² <#581 (comment)>
tsibley
force-pushed
the
trs/relax-redis-requirement
branch
from
3945b21
to
2f86177
Compare
victorlin
reviewed
Oct 26, 2023
victorlin
reviewed
Oct 26, 2023
This is made possible by the previous commit which switched the non-Redis fallback from an in-memory store to an on-disk SQLite store. As a nice side-effect, we can now set REDIS_REQUIRED=true in development mode (i.e. not the default) to more easily test the production mode code path. Related-to: <nextstrain/private#94>
tsibley
force-pushed
the
trs/relax-redis-requirement
branch
from
2f86177
to
64f7d92
Compare
6 tasks
victorlin
approved these changes
Oct 31, 2023
tsibley
commented
Nov 1, 2023
| * ² <https://devcenter.heroku.com/articles/app-json-schema> | ||
| * ³ <https://devcenter.heroku.com/articles/github-integration-review-apps> | ||
| */ | ||
| export const REDIS_REQUIRED = fromEnvOrConfig("REDIS_REQUIRED", PRODUCTION && !REVIEW_APP); |
There was a problem hiding this comment.
Welp, this doesn't work at all when setting REDIS_REQUIRED=false in the environment or "REDIS_REQUIRED": false in the config because of the use of || in fromEnvOrConfig(). orz
I tested the functionality and it worked! but it only appeared to work because I was testing against a previous version of the code which provided a default of null instead of PRODUCTION && !REVIEW_APP. In my testing, the production config set REDIS_REQUIRED=true, which worked, and the testing config set it to false, which worked, but the actual value was resolved to null (the default). This had the same effect as false and so masked the problematic use of ||. When I changed the default, the behaviour changed, and I didn't re-test. Should have written proper tests, but I didn't because then we need an actual REDIS_URL for the connection. We could provide one in CI I suppose…
tsibley
added a commit
that referenced
this pull request
Nov 2, 2023
…instead of considering them missing. The values considered missing are now more clear: 1. undefined (i.e. not present) 2. null (i.e. present but explicitly null) 3. "" (i.e. present but empty, particularly for env vars) This allows falsy values¹ like the number 0 and boolean false to be configurable values², as expected to be for OIDC_IAT_BACKDATED_BY and REDIS_REQUIRED. For example, setting REDIS_REQUIRED=false is now functional as intended by "Allow and document how to run safely in production mode without Redis" (64f7d92). And setting OIDC_IAT_BACKDATED_BY=0 in the environment now overrides any value from the config file instead of being ignored. Related-to: <#739> ¹ <https://developer.mozilla.org/en-US/docs/Glossary/Falsy> ² Other falsy values like NaN and 0n are also now considered non-missing, but there's no way to express those in our current config sources, which permit only strings or JSON values. We may find ourselves wanting to treat NaN as missing too if config sources change.
1 task
tsibley
added a commit
that referenced
this pull request
Nov 2, 2023
…instead of considering them missing. The values considered missing are now more clear: 1. undefined (i.e. not present) 2. null (i.e. present but explicitly null) 3. "" (i.e. present but empty, particularly for env vars) This allows falsy values¹ like the number 0 and boolean false to be configurable values², as expected to be for OIDC_IAT_BACKDATED_BY and REDIS_REQUIRED. For example, setting REDIS_REQUIRED=false is now functional as intended by "Allow and document how to run safely in production mode without Redis" (64f7d92). And setting OIDC_IAT_BACKDATED_BY=0 in the environment now overrides any value from the config file instead of being ignored. Related-to: <#739> ¹ <https://developer.mozilla.org/en-US/docs/Glossary/Falsy> ² Other falsy values like NaN and 0n are also now considered non-missing, but there's no way to express those in our current config sources, which permit only strings or JSON values. We may find ourselves wanting to treat NaN as missing too if config sources change.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See commit messages.
Related-to: https://github.com/nextstrain/private/issues/94
Checklist