feat: topic in-page navigation via sefaria:bootstrap-url#3377
Open
yodem wants to merge 5 commits into
Open
Conversation
…L; retire slack-webhook secret [sc-44609] SLACK_URL was injected twice: an explicit `env` from the dedicated slack-webhook secret (sefaria.secrets.slackWebhook) and the SLACK_URL key bundled in local-settings-secrets (via envFrom). In Kubernetes an explicit `env` always wins over an envFrom key of the same name, so SLACK_URL overrides set in an environment's / cauldron's local-settings-secrets were silently ignored, leaving pods serving a stale webhook (invalid_token). Make local-settings-secrets the single source of truth and retire the dedicated slack-webhook secret entirely: - Remove the redundant explicit SLACK_URL env from the pods that already mount local-settings-secrets (web, task, reindex-elasticsearch, mongo-backup, postgres-packup) so the envFrom value takes effect. - Migrate the blue-green deploy notifier (analysistemplate/rollout-complete) off the slack-webhook secret: SLACK_URL now comes from local-settings-secrets and the target channel from a new SLACK_CHANNEL key there. Both are optional (the curl is also `|| /bin/true`) so the post-promotion notification never blocks a rollout. - Delete the slack-webhook secret template, the sefaria.secrets.slackWebhook helper, and the secrets.slackWebhook values stanza. Companion infra change (separate PR): add the SLACK_CHANNEL key to local-settings-secrets (prod + dev) and remove the orphaned slack-webhook-* secrets. SLACK_URL already exists in those secrets. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…localSettings ref [sc-44609] Addresses Copilot review on PR #3369: - postgres-packup: the pgdump-uploader container (which runs upload-dumps.sh → `curl ... ${SLACK_URL}` under `set -e`) has no envFrom, so dropping the explicit SLACK_URL env left it unset and would fail the backup job. Restore SLACK_URL from the configured local-settings secret. - mongo-backup: upload-dumps.sh (mongo) does not use SLACK_URL at all — the previous env was dead. Keep it removed; correct the misleading comment. - rollout-complete analysistemplate: reference {{ .Values.secrets.localSettings.ref }} (prod: local-settings-secrets-production, where SLACK_URL/SLACK_CHANNEL live) instead of the hardcoded local-settings-secrets-{{ deployEnv }} pattern. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nnel [sc-44609] The deploy notifier uses a Slack incoming webhook (SLACK_URL), which is bound to a channel at creation; the payload channel override is redundant (and ignored by app-based webhooks). Remove the CHANNEL env and the channel field from the payload, so no SLACK_CHANNEL key is needed in local-settings-secrets. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The explanatory comments read well in the PR diff but are noise in the long-lived chart after merge. Remove them; keep two short present-tense notes where the behavior is non-obvious (pg-backup uploader's explicit SLACK_URL, and the deliberate absence of a channel override). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📊 Code Quality Score: 14/100
Was this score accurate? 👍 Yes · 👎 No Scored by GitVelocity · How are scores calculated? |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Sefaria Helm chart to remove the dedicated slackWebhook secret/template and to source SLACK_URL from the existing secrets.localSettings.ref secret (with some components no longer explicitly injecting SLACK_URL). Note: the PR title/description references chatbot topic in-page navigation, but the actual diff is entirely Helm/Slack-secret related.
Changes:
- Remove
secrets.slackWebhookvalues + the slack webhook Secret template/helper. - Stop explicitly injecting
SLACK_URLinto web/tasks and some cronjobs; rely onenvFromlocal settings where present. - Update rollout-complete and postgres backup cronjob to read
SLACK_URLfromsecrets.localSettings.ref(rollout-complete usesoptional: true).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| helm-chart/sefaria/values.yaml | Removes secrets.slackWebhook values block; chart configuration surface changes. |
| helm-chart/sefaria/templates/secret/slack-webhook.yaml | Deletes the Slack webhook Secret template. |
| helm-chart/sefaria/templates/rollout/web.yaml | Removes explicit SLACK_URL env var from web rollout. |
| helm-chart/sefaria/templates/rollout/task.yaml | Removes explicit SLACK_URL env var from tasks rollout. |
| helm-chart/sefaria/templates/cronjob/reindex-elasticsearch.yaml | Removes explicit SLACK_URL env var from reindex cronjob. |
| helm-chart/sefaria/templates/cronjob/postgres-packup.yaml | Switches SLACK_URL secretKeyRef to secrets.localSettings.ref / SLACK_URL. |
| helm-chart/sefaria/templates/cronjob/mongo-backup.yaml | Removes explicit SLACK_URL env var from mongo backup uploader. |
| helm-chart/sefaria/templates/analysistemplate/rollout-complete.yaml | Removes channel override from Slack payload; sources SLACK_URL from local settings secret (optional). |
| helm-chart/sefaria/templates/_helpers.tpl | Removes sefaria.secrets.slackWebhook helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
392
to
396
| ref: | ||
| # data: | ||
| # key: | ||
| # serviceAccount: | ||
| # This is used in mongobackup & reindex-elasticsearch cron jobs. | ||
| slackWebhook: | ||
| # If you're using a reference to an existing secret then the data: section | ||
| # should be commented out and vice-versa. | ||
| ref: | ||
| # data: | ||
| webpageCronjob: |
Comment on lines
77
to
+81
| - name: SLACK_URL | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ template "sefaria.secrets.slackWebhook" . }} | ||
| key: slack-webhook | ||
| name: {{ .Values.secrets.localSettings.ref }} | ||
| key: SLACK_URL |
| # should be commented out and vice-versa. | ||
| ref: | ||
| # data: | ||
| webpageCronjob: |
…nto feat/topic-bootstrap-url-navigation
This file contains hidden or 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
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.
Summary
Enables in-page navigation for topic links dispatched via
sefaria:bootstrap-urlfrom the AI chatbot.The fix for the
bootstrapUrlcrash on non-ref URLs (topics, profiles, etc.) is already in master (bb7cf2775). This branch + cauldron is for testing chatbot topic navigation end-to-end.Test
Load any Sefaria page with
?chatbot_version=149, ask about a topic, click a topic link — should navigate in-page.🤖 Generated with Claude Code