github-actions[bot]
commented
2 months ago QA Summary
| QA Check | Result |
|---|---|
| 🌐 Client Tests | ✅ |
| 🔗 Server Tests | ✅ |
| 🤝 E2E Tests | ✅ |
| 📏 ESLint | ✅ |
| 🧹 TFLint | ✅ |
Open masimons opened 2 months ago
github-actions[bot]
commented
2 months ago | QA Check | Result |
|---|---|
| 🌐 Client Tests | ✅ |
| 🔗 Server Tests | ✅ |
| 🤝 E2E Tests | ✅ |
| 📏 ESLint | ✅ |
| 🧹 TFLint | ✅ |
github-actions[bot]
commented
2 months ago | Step | Result |
|---|---|
| 🖌 Terraform Format & Style | ✅ |
| ⚙️ Terraform Initialization | ✅ |
| 🤖 Terraform Validation | ✅ |
| 📖 Terraform Plan | ✅ |
Hint: If "Terraform Format & Style" failed, run terraform fmt -recursive from the terraform/ directory and commit the results.
Pusher: @lsr-explore, Action: pull_request_target, Workflow: Continuous Integration
masimons
commented
1 month ago @TylerHendrickson thanks for checking this out, I missed this comment. Out of curiosity, do you have an example off the top of your head where we use an env var in that way? I was looking into how we use feature flags, but it seems that those are scoped to the client side?
TylerHendrickson
commented
1 month ago @masimons re
Out of curiosity, do you have an example off the top of your head where we use an env var in that way? I was looking into how we use feature flags, but it seems that those are scoped to the client side?
The implementation details for #3060 provide some instructions for implementing an API-side feature flag using a SHARE_TERMINOLOGY_ENABLED environment variable (plus a client-side feature flag, which doesn't apply here). This was implemented by PR #3147.
Fundamentally, this sort of thing would entail the following:
packages/server/.env.example.api_container_environment input variable in terraform/staging.tfvars.terraform/prod.tfvars.I'd recommend that an environment variable that provides a binary feature flag be evaluated as a string using strict equality, e.g. my_feature_is_enabled = process.env.MY_FEATURE_ENABLED === 'true'.
lsr-explore
commented
3 weeks ago @masimons I really like how this implementation parameterizes the flag in
src/db/index.jsand allows the caller to decide whether to pull in the toggle from env var or something else. This is looking good overall!I did notice in local testing that setting
SHOW_FORECASTED_GRANTS=falseinpackages/server/.envdoesn't seem to have the desired effect (I could still see forecasted grants in search results), although when the env var is altogether missing, forecasted grants are hidden like we'd expect.I believe this is because the
packages/server/db/index.jsfunctions are expecting a true boolean value (or elseundefined, so that the arg default offalsein the function signature is used), but when a value is provided fromprocess.env.SHOW_FORECASTED_GRANTS, it's a string rather than a boolean, and!"false"is evaluating totrue. I think the simplest way to resolve this is to have callers explicitly check whetherprocess.env.SHOW_FORECASTED_GRANTS === "true", which I provided in a comment suggestion.
@TylerHendrickson , @masimons - I've accepted Ty's changes - this should be ready for review
lsr-explore
commented
2 weeks ago @lsr-explore Thanks for the updates. I think we need the strict
=== 'true'comparison for all usages of the environment variable, so I added a few additional comment suggestions to address those as well. I think that I covered all of them, but feel free to double check me on that 🙂
Sorry that I missed this @TylerHendrickson . I applied the changes and did another sanity check of the code. I noticed a couple additional places where the flag needed to be sent and added a commit.
I noticed that the db.getGrantsInterested didn't have this check. This would only be an issue if we needed to turn off grants after enabling it and user had clicked follow.
Do you think we need the check on getGrantsInterested?
masimons
commented
2 weeks ago I noticed that the db.getGrantsInterested didn't have this check. This would only be an issue if we needed to turn off grants after enabling it and user had clicked follow.
Do you think we need the check on getGrantsInterested?
That's a fair point - I don't know if that particular case came up when we were thinking about this
Ticket #3213
Description
Hide forecasted grants on production during development and testing.
Screenshots / Demo Video
Testing
Automated and Unit Tests
Manual tests for Reviewer
Checklist