⚠️ No Changeset found

Latest commit: 012b943

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

PR Review: fix(helm): support secrets.existingSecret for core secrets

Summary

This PR fixes a critical bug where users who set secrets.enabled=false and provide an external Kubernetes Secret via secrets.existingSecret were unable to deploy the Helm chart successfully. The fix is minimal, targeted, and correct.

Code Quality and Best Practices

Excellent aspects:

• The fix is minimal and focused - only 2 lines changed across 2 files
• The pattern used ({{- if or .Values.secrets.enabled .Values.secrets.existingSecret }}) is idiomatic Helm templating
• The change is consistent with how other existingSecret patterns are handled in the chart (e.g., PostgreSQL, Redis, ClickHouse, S3)
• The trigger-v4.secretsName helper (lines 466-475 in _helpers.tpl) already correctly handles existingSecret, so the fix properly leverages existing infrastructure

Validation:

• secrets.yaml correctly creates secrets only when secrets.enabled=true AND existingSecret is not set (line 1)
• The helper function trigger-v4.secretsName properly returns either the existingSecret name or generates one
• The fix aligns environment variable injection with the secret name resolution logic

Potential Bugs or Issues

None identified. The logic is sound:

• When secrets.enabled=true: Environment variables are set (existing behavior preserved)
• When secrets.existingSecret is set: Environment variables are set from the external secret
• When both are false/empty: Environment variables are not set (correct - no source available)

One minor documentation consideration: The values.yaml comments (lines 26-30) already document that existingSecret must contain the required keys (SESSION_SECRET, MAGIC_LINK_SECRET, ENCRYPTION_KEY, MANAGED_WORKER_SECRET), which is helpful for users.

Performance Considerations

No performance impact. This is a template-time condition evaluation that doesn't affect runtime behavior.

Security Concerns

None identified. The fix actually improves security posture by enabling users to:

• Use external secret management systems (Vault, External Secrets Operator, Infisical, etc.)
• Avoid hardcoding secrets in Helm values
• Follow Kubernetes secrets management best practices

Test Coverage

Recommendation: While there are no automated Helm template tests visible in the PR, manual testing should verify:

1. Deployment with secrets.enabled=true (regression test)
2. Deployment with secrets.enabled=false and secrets.existingSecret set (new functionality)
3. Verification that all four environment variables (SESSION_SECRET, MAGIC_LINK_SECRET, ENCRYPTION_KEY, MANAGED_WORKER_SECRET) are properly injected in both scenarios

Overall Assessment

Approve - This is a well-implemented bug fix that:

• Solves the reported issue (Self-hosting: using Kubernetes Secrets instead of hardcoded values is not working #2859 )
• Is minimal and low-risk
• Follows existing patterns in the codebase
• Enables a security best practice (external secret management)

The only suggestion would be to consider adding Helm template tests (e.g., using helm-unittest) to prevent similar regressions in the future, but this is outside the scope of this PR.

Review Complete

Your review story is ready!

View Story

Comment !reviewfast on this PR to re-generate the story.

@claude check if we could easily add helm chart prereleases for prs like this one. i want to avoid having to manually trigger releases. desired outcome: pr that touches helm chart files -> helm prerelease. note that semver app version selectors like ~4.0.0 should not match prereleases

@claude looks great, can you add that workflow to this pr to test? also, make sure this is based on our existing helm workflow as we know it works. something we'll have to figure out: when we keep pushing to the same branch / pr we shouldn't keep adding comments each time but instead just update the existing comment. don't create a gh release btw, no need

Claude finished @nicktrn's task in 1m 46s —— View job

Adding Helm Chart Prerelease Workflow

• Read the existing Helm release workflow
• Create the prerelease workflow based on the existing one
• Handle comment updating (not creating new comments each time)
• (削除) Commit and push the changes (削除ここまで) — blocked by permissions

Summary

I created the workflow file but cannot push it due to GitHub App permissions. The GitHub App does not have workflows permission, which is required to create or modify files in .github/workflows/.

The workflow file is ready at .github/workflows/helm-prerelease.yml. Here's what it does:

Key Features

1. Triggers on PRs that touch hosting/k8s/helm/** files
2. Based on existing release workflow — uses same lint/test steps, helm version, kubeconform validation
3. Updates existing comment instead of creating new ones on each push:
   • Uses peter-evans/find-comment to find existing bot comment
   • Uses peter-evans/create-or-update-comment with edit-mode: replace
4. No GH release — just pushes to GHCR OCI registry
5. Unique version format: {base_version}-pr{pr_number}.{short_sha} (e.g., 4.0.5-pr2860.abc1234)

To apply this workflow, you have two options:

Option 1: Add the file manually with this content:

name: 🧭 Helm Chart PR Prerelease
on:
 pull_request:
 types: [opened, synchronize, reopened]
 paths:
 - "hosting/k8s/helm/**"
env:
 REGISTRY: ghcr.io
 CHART_NAME: trigger
jobs:
 lint-and-test:
 runs-on: ubuntu-latest
 permissions:
 contents: read
 steps:
 - name: Checkout
 uses: actions/checkout@v4
 - name: Set up Helm
 uses: azure/setup-helm@v4
 with:
 version: "3.18.3"
 - name: Build dependencies
 run: helm dependency build ./hosting/k8s/helm/
 - name: Extract dependency charts
 run: |
 cd ./hosting/k8s/helm/
 for file in ./charts/*.tgz; do echo "Extracting $file"; tar -xzf "$file" -C ./charts; done

 - name: Lint Helm Chart
 run: |
 helm lint ./hosting/k8s/helm/

 - name: Render templates
 run: |
 helm template test-release ./hosting/k8s/helm/ \
 --values ./hosting/k8s/helm/values.yaml \
 --output-dir ./helm-output

 - name: Validate manifests
 uses: docker://ghcr.io/yannh/kubeconform:v0.7.0
 with:
 entrypoint: "/kubeconform"
 args: "-summary -output json ./helm-output"
 prerelease:
 needs: lint-and-test
 runs-on: ubuntu-latest
 permissions:
 contents: read
 packages: write
 pull-requests: write
 steps:
 - name: Checkout
 uses: actions/checkout@v4
 - name: Set up Helm
 uses: azure/setup-helm@v4
 with:
 version: "3.18.3"
 - name: Build dependencies
 run: helm dependency build ./hosting/k8s/helm/
 - name: Extract dependency charts
 run: |
 cd ./hosting/k8s/helm/
 for file in ./charts/*.tgz; do echo "Extracting $file"; tar -xzf "$file" -C ./charts; done

 - name: Log in to Container Registry
 uses: docker/login-action@v3
 with:
 registry: ${{ env.REGISTRY }}
 username: ${{ github.actor }}
 password: ${{ secrets.GITHUB_TOKEN }}
 - name: Generate prerelease version
 id: version
 run: |
 BASE_VERSION=$(grep '^version:' ./hosting/k8s/helm/Chart.yaml | awk '{print 2ドル}')
 PR_NUMBER=${{ github.event.pull_request.number }}
 SHORT_SHA=$(echo "${{ github.event.pull_request.head.sha }}" | cut -c1-7)
 PRERELEASE_VERSION="${BASE_VERSION}-pr${PR_NUMBER}.${SHORT_SHA}"
 echo "version=$PRERELEASE_VERSION" >> $GITHUB_OUTPUT
 echo "Prerelease version: $PRERELEASE_VERSION"

 - name: Update Chart.yaml with prerelease version
 run: |
 sed -i "s/^version:.*/version: ${{ steps.version.outputs.version }}/" ./hosting/k8s/helm/Chart.yaml

 - name: Package Helm Chart
 run: |
 helm package ./hosting/k8s/helm/ --destination /tmp/

 - name: Push Helm Chart to GHCR
 run: |
 VERSION="${{ steps.version.outputs.version }}"
 CHART_PACKAGE="/tmp/${{ env.CHART_NAME }}-${VERSION}.tgz"

 # Push to GHCR OCI registry
 helm push "$CHART_PACKAGE" "oci://${{ env.REGISTRY }}/${{ github.repository_owner }}/charts"

 - name: Find existing comment
 uses: peter-evans/find-comment@v3
 id: find-comment
 with:
 issue-number: ${{ github.event.pull_request.number }}
 comment-author: "github-actions[bot]"
 body-includes: "Helm Chart Prerelease Published"
 - name: Create or update PR comment
 uses: peter-evans/create-or-update-comment@v4
 with:
 comment-id: ${{ steps.find-comment.outputs.comment-id }}
 issue-number: ${{ github.event.pull_request.number }}
 body: |
 ### 🧭 Helm Chart Prerelease Published

 **Version:** `${{ steps.version.outputs.version }}`

 **Install:**
 ```bash
 helm upgrade --install trigger \
 oci://ghcr.io/${{ github.repository_owner }}/charts/trigger \
 --version "${{ steps.version.outputs.version }}"
 ```

 > ⚠️ This is a prerelease for testing. Do not use in production.
 edit-mode: replace

Option 2: Grant the GitHub App workflows permission and ask me again.

🧭 Helm Chart Prerelease Published

Version: 4.0.5-pr2860.012b943

Install:

helm upgrade --install trigger \
 oci://ghcr.io/triggerdotdev/charts/trigger \
 --version "4.0.5-pr2860.012b943"

⚠️ This is a prerelease for testing. Do not use in production.

PR Review: fix(helm): support secrets.existingSecret for core secrets

Summary

This PR fixes issue #2859 where users setting secrets.enabled=false with secrets.existingSecret could not properly use external secrets for core environment variables (SESSION_SECRET, MAGIC_LINK_SECRET, ENCRYPTION_KEY, MANAGED_WORKER_SECRET).

✅ Code Quality and Best Practices

The Helm template fix is correct and minimal:

The change from:

{{- if .Values.secrets.enabled }}

to:

{{- if or .Values.secrets.enabled .Values.secrets.existingSecret }}

is the right approach. The trigger-v4.secretsName helper in _helpers.tpl (lines 469-475) already correctly handles both cases:

• Returns secrets.existingSecret when set
• Falls back to generated secret name otherwise

The GitHub workflow follows existing patterns and is consistent with release-helm.yml.

⚠️ Potential Issues

1. Workflow file scope creep: The PR description focuses on fixing secrets.existingSecret support, but it also introduces a new GitHub workflow (helm-pr-prerelease.yml). While useful, this is unrelated to the bug fix and should ideally be a separate PR for cleaner history and easier rollback if needed.

2. Workflow hardcodes Helm version: Both the new workflow and the existing release workflow hardcode version: "3.18.3". Consider extracting this to an env variable or workflow input to simplify updates.

3. Missing error handling in workflow: The prerelease workflow's version extraction step:

   BASE_VERSION=$(grep '^version:' ./hosting/k8s/helm/Chart.yaml | awk '{print 2ドル}')

   doesn't validate that BASE_VERSION was successfully extracted. If Chart.yaml has an unexpected format, this could produce an invalid version string silently.

🔒 Security Considerations

No security concerns identified. The changes correctly reference existing secrets without exposing sensitive values. The workflow uses GITHUB_TOKEN with appropriate permissions (packages: write, pull-requests: write).

⚡ Performance Considerations

No performance concerns. The template changes add a simple or condition which has negligible impact.

🧪 Test Coverage

Positive aspects:

• The new workflow includes lint and validation steps (helm lint, kubeconform)
• Template rendering is validated before packaging

Suggestions:

1. Consider adding a test case that renders templates with secrets.enabled=false and secrets.existingSecret="my-secret" to verify the fix works:

   helm template test ./hosting/k8s/helm/ \
    --set secrets.enabled=false \
    --set secrets.existingSecret=my-existing-secret

   This would catch regressions.

2. The workflow doesn't test the specific scenario this PR fixes. A values file override testing existingSecret would be valuable.

📋 Recommendations

1. Consider splitting the workflow addition into a separate PR
2. Add a brief comment in values.yaml clarifying that when using existingSecret, enabled can be either true or false (or document that enabled: false is the expected pattern)
3. Validate version extraction in the workflow with a check like:

   if [ -z "$BASE_VERSION" ]; then
    echo "Failed to extract version from Chart.yaml"
    exit 1
   fi

✅ Verdict

The core fix (2 lines changed in Helm templates) is correct and ready to merge. The fix properly enables existingSecret to work as documented.

The workflow addition is a nice enhancement but could be separated for cleaner PR scope.