Use dummy CAPTCHA site key in default config #3332
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3332 +/- ##
==========================================
- Coverage 74.73% 74.69% -0.04%
==========================================
Files 280 280
Lines 10744 10743 -1
Branches 1298 1297 -1
==========================================
- Hits 8029 8025 -4
- Misses 2350 2353 +3
Partials 365 365
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Reviewed 3 of 4 files at r2, all commit messages.
Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @imnasnainaec)
deploy/scripts/setup_files/profiles/dev.yaml line 17 at r2 (raw file):
# has dummy secret keys for development and testing; options are # invisible pass, invisible fail, visible pass, visible fail, forced interaction configCaptchaSiteKey: "1x00000000000000000000AA" # visible pass
This needs to stay. If it is removed, then when you build for your local kubernetes cluster it set the site key to "". As the value said in the file ./deploy/helm/thecombine/charts/frontend/values.yaml, the value needs to be defined in the profiles.
In general, a blank value should be acceptable for any of the environment variables and should not be interpreted as though the variable was not specified.
Code quote:
frontend:
# https://developers.cloudflare.com/turnstile/troubleshooting/testing/
# has dummy secret keys for development and testing; options are
# invisible pass, invisible fail, visible pass, visible fail, forced interaction
configCaptchaSiteKey: "1x00000000000000000000AA" # visible pass
There was a problem hiding this comment.
Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on @imnasnainaec and @jmgrady)
deploy/scripts/setup_files/profiles/dev.yaml line 17 at r2 (raw file):
You make a good point with
a blank value should be acceptable for any of the environment variables and should not be interpreted as though the variable was not specified.
Does the change to deploy/helm/thecombine/charts/frontend/templates/env-frontend-configmap.yaml address that?
There was a problem hiding this comment.
Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on @imnasnainaec)
deploy/scripts/setup_files/profiles/dev.yaml line 17 at r2 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
You make a good point with
a blank value should be acceptable for any of the environment variables and should not be interpreted as though the variable was not specified.
Does the change to
deploy/helm/thecombine/charts/frontend/templates/env-frontend-configmap.yamladdress that?
No. It breaks the deployment. Please revert this change and the change in env-frontend-config.yaml.
There was a problem hiding this comment.
Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on @imnasnainaec)
deploy/scripts/setup_files/profiles/dev.yaml line 17 at r2 (raw file):
Previously, jmgrady (Jim Grady) wrote…
No. It breaks the deployment. Please revert this change and the change in
env-frontend-config.yaml.
It also removes the ability to set the site key to "" which may be desirable when the CAPTCHA is not enabled.
There was a problem hiding this comment.
Reviewed 1 of 4 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @imnasnainaec)
deploy/helm/thecombine/charts/frontend/templates/env-frontend-configmap.yaml line 14 at r3 (raw file):
{{- if .Values.configCaptchaSiteKey }} CONFIG_CAPTCHA_SITE_KEY: {{ .Values.configCaptchaSiteKey | quote }} {{- end }}
Please remove these changes. They break the deployment. In addition, they add complexity and adding similar code to the deployment would add more complexity.
Code quote:
{{- if .Values.configCaptchaSiteKey }}
CONFIG_CAPTCHA_SITE_KEY: {{ .Values.configCaptchaSiteKey | quote }}
{{- end }}
There was a problem hiding this comment.
Reviewable status: 3 of 5 files reviewed, 2 unresolved discussions (waiting on @jmgrady)
deploy/helm/thecombine/charts/frontend/templates/env-frontend-configmap.yaml line 14 at r3 (raw file):
Previously, jmgrady (Jim Grady) wrote…
Please remove these changes. They break the deployment. In addition, they add complexity and adding similar code to the deployment would add more complexity.
Retracted based on in-Slack conversation.
deploy/scripts/setup_files/profiles/dev.yaml line 17 at r2 (raw file):
Previously, jmgrady (Jim Grady) wrote…
It also removes the ability to set the site key to "" which may be desirable when the CAPTCHA is not enabled.
Retracted based on in-Slack conversation.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @imnasnainaec)
This change is