Rewrite deploy ecamp3 #8025
Rewrite deploy ecamp3 #8025
Conversation
…figmap.yaml Else the sentry environment is not set which gets. This was never tested when we added this feature.
BacLuc
force-pushed
the
rewrite-deploy-ecamp3
branch
from
050f857 to
f7773e0
Compare
|
@coderabbitai review this pr |
|
@BacLuc I'll review this PR which involves rewriting the ecamp3 deployment to use helmfile. This includes changes to allow frontend deployment without sentry DSN, merging deployment configurations, and transitioning from values.yaml to values.yaml.gotmpl format. ✅ Actions performedReview triggered.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR replaces helm/Helm upgrade flows with a Helmfile-driven deployment pipeline. It introduces a reusable ecamp3 deployment workflow, a composite action to install helmfile, new Helmfile chart templates/config, and scripts to build images, generate JWTs, and deploy via diff/deploy modes. GitHub workflows are updated to pass an action input and target the new reusable workflow; legacy reusable workflows and old values/env examples are removed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub Actions (caller)
participant Reusable as reusable-ecamp3-deployment.yml
participant SetupHF as setup-helmfile action
participant Repo as Repo Checkout
participant Kube as Kubernetes Cluster
participant PR as GitHub PR
Dev->>GH: workflow_dispatch (inputs: action=diff|deploy)
GH->>Reusable: workflow_call (name, env, sha, action, ...)
activate Reusable
Reusable->>Repo: actions/checkout@vX (sha)
Reusable->>Reusable: Merge secrets/vars -> .helm/ecamp3/env.yaml
Reusable->>SetupHF: Install helmfile
Reusable->>Reusable: Generate values via helmfile write-values
alt action == diff
Reusable->>Kube: kubectl diff (helm template output)
else action == deploy
Reusable->>Kube: helm upgrade --install (values.yaml)
end
opt env == feature-branch
Reusable->>PR: Post progress/success/failure comments
end
deactivate Reusable
sequenceDiagram
autonumber
participant Wrapper as .helm/deploy-to-cluster.sh
participant Deploy as .helm/ecamp3/deploy.sh
participant Helmfile as helmfile
participant Helm as helm
participant Kubectl as kubectl
Wrapper->>Wrapper: Read NAME from ecamp3/env.yaml
Wrapper->>Deploy: Call with ACTION (default: diff)
Deploy->>Helmfile: write-values -> values.yaml
alt ACTION=deploy
Deploy->>Helm: upgrade --install ecamp3-$NAME -f values.yaml
else ACTION=diff
Deploy->>Helm: template (no hooks, skip tests)
Deploy->>Kubectl: diff -f rendered manifests
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (36)
.helm/ecamp3/.gitignore (1)
1-3: Git-ignoring env/materials locally is good; consider ignoring common local variants tooNice safeguard to prevent accidental commits of environment-specific files. Consider also ignoring common local variants to avoid footguns during manual runs.
Apply this diff to extend ignore patterns:
.env /env.yaml /values.yaml +/# local overrides and variants +/env.local.yaml +/env.*.yaml +/values.local.yaml.helm/ecamp3/.helmignore (1)
26-32: Deduplicate .env and consider excluding env.example.yaml from chart packaging
- .env appears twice (Line 26 and Line 28). Remove the duplicate.
- env.example.yaml is helpful in the repo but shouldn’t be packaged into the chart. Exclude it to keep the release artifact minimal.
Apply this diff:
.env .env-example -.env /deploy.sh /values.yaml /values.yaml.gotmpl /env.yaml +/# repo-only example, not needed in packaged chart +/env.example.yaml.helm/ecamp3/templates/frontend_configmap.yaml (2)
14-20: Clarify intent: Should SENTRY_ENVIRONMENT be omitted when DSN is absent?The PR objective says “allow deploying the frontend without a Sentry DSN … so the Sentry environment can be omitted.” The current change sets SENTRY_ENVIRONMENT in both branches. If the intended behavior is to omit it when DSN is missing, keep SENTRY_ENVIRONMENT only in the DSN-present branch or set it to null in the else.
Two alternatives:
- Omit SENTRY_ENVIRONMENT when DSN is missing:
{{- if .Values.frontend.sentryDsn }} SENTRY_FRONTEND_DSN: '{{ .Values.frontend.sentryDsn }}', SENTRY_ENVIRONMENT: '{{ .Values.domain }}', {{- else }} - SENTRY_FRONTEND_DSN: null, - SENTRY_ENVIRONMENT: '{{ .Values.domain }}', + SENTRY_FRONTEND_DSN: null, {{- end }}
- Or keep the key but make it explicit null:
{{- else }} SENTRY_FRONTEND_DSN: null, - SENTRY_ENVIRONMENT: '{{ .Values.domain }}', + SENTRY_ENVIRONMENT: null, {{- end }}
11-21: Guard against missing .Values.domainIf domain is required, fail fast with a clear error instead of rendering an empty/invalid JS value. Helm supports required.
Apply this diff to wrap domain usage:
- API_ROOT_URL: '{{ include "api.url" . }}', + API_ROOT_URL: '{{ include "api.url" . }}', COOKIE_PREFIX: '{{ include "api.cookiePrefix" . }}', PRINT_URL: '{{ include "print.url" . }}', {{- if .Values.frontend.sentryDsn }} - SENTRY_FRONTEND_DSN: '{{ .Values.frontend.sentryDsn }}', - SENTRY_ENVIRONMENT: '{{ .Values.domain }}', + SENTRY_FRONTEND_DSN: '{{ .Values.frontend.sentryDsn }}', + SENTRY_ENVIRONMENT: '{{ required "values.domain must be set" .Values.domain }}', {{- else }} SENTRY_FRONTEND_DSN: null, - SENTRY_ENVIRONMENT: '{{ .Values.domain }}', + SENTRY_ENVIRONMENT: {{- if .Values.domain -}}'{{ .Values.domain }}'{{- else -}}null{{- end -}}, {{- end }}.helm/generate-jwt-values.sh (4)
2-5: Redundant set -e and tighten error handling/cleanup
- set -e is duplicated.
- Add a trap to ensure TMP_DIR is cleaned on error as well.
Apply this diff:
-#!/bin/bash -set -euo pipefail - -set -e +#!/bin/bash +set -euo pipefail + +# Cleanup temp dir even on error +cleanup() { [[ -n "${TMP_DIR:-}" && -d "$TMP_DIR" ]] && rm -rf "$TMP_DIR"; } +trap cleanup EXIT
6-8: Remove unused REPO_DIR and avoid portability pitfalls with realpathREPO_DIR is unused (SC2034). Also, realpath isn’t universal on all runners. dirname + cd + pwd is more portable.
Apply this diff:
-SCRIPT_DIR=$(realpath "$(dirname "$0")") -REPO_DIR=$(realpath "$SCRIPT_DIR"/..) +SCRIPT_DIR="$(cd -- "$(dirname -- "$0")" && pwd -P)"
11-14: Passphrase generation and key permissions
- uuidgen may be missing on some systems; provide a fallback.
- Ensure private material is created with restrictive permissions.
Apply this diff:
-TMP_DIR=$(mktemp -d) +TMP_DIR=$(mktemp -d) +umask 077 - -jwt_passphrase=$(uuidgen) +jwt_passphrase=$(uuidgen 2>/dev/null || openssl rand -hex 32) echo -n "$jwt_passphrase" | openssl genpkey -out "$TMP_DIR"/private.pem -pass stdin -aes256 -algorithm rsa -pkeyopt rsa_keygen_bits:4096 echo -n "$jwt_passphrase" | openssl pkey -in "$TMP_DIR"/private.pem -passin stdin -out "$TMP_DIR"/public.pem -pubout
20-24: Quote paths and use a name that reflects content (it’s YAML/JSON env, not “json_file”)Unquoted paths can break with spaces. Also, the variable name is misleading.
Apply this diff:
-json_file="$SCRIPT_DIR/ecamp3/env.yaml" +env_file="$SCRIPT_DIR/ecamp3/env.yaml" -if [ ! -f $json_file ]; then - cp "$SCRIPT_DIR/ecamp3/env.example.yaml" $json_file +if [ ! -f "$env_file" ]; then + cp "$SCRIPT_DIR/ecamp3/env.example.yaml" "$env_file" fi.helm/README.md (4)
18-27: Fix typos, link syntax, and clarify wording in Setup section
- Close the Markdown link and fix grammar.
- Be explicit about creating env.yaml only when missing.
Apply this diff:
-If you don't have JWT Passphrase, public and private key yet, you have to run: +If you don't have a JWT passphrase, public key, and private key yet, run: -This copies [env.example.yaml](ecamp3/env.example.yaml) to [env.yaml](ecamp3/env.yaml) -if not exists and sets the jwt values. +This copies [env.example.yaml](ecamp3/env.example.yaml) to [env.yaml](ecamp3/env.yaml) +if it does not exist and sets the JWT values. -Then you have to set the values in [env.yaml](ecamp3/env.yaml which are not set to any value. -(e.g. POSTGRES_URL). +Then set the values in [env.yaml](ecamp3/env.yaml) that are not populated +(e.g., POSTGRES_URL).
9-14: Add missing prerequisites (git) and optionally pin tool versionsvalues.yaml.gotmpl executes git and date; without git installed, write-values will fail. Consider also documenting tested versions.
- jq - kubectl (with a kubeconfig for the cluster you want to deploy to) - helm - helmfile - docker (with a public repository to push images to) - openssl + - git + +Recommended versions (tested): +- Helm ≥ 3.13 +- Helmfile = v1.1.3 (matches CI) +- kubectl matching your cluster minor version
29-33: Call out docker login before buildingAvoid build/push failures when the repo is private or rate-limited.
## Build images ```shell +docker login ./build-images.sh--- `16-28`: **Document required keys in env.yaml up front** Make explicit which values are required by the templates to save a round-trip for newcomers. ```diff ## Setup @@ -Then you have to set the values in [env.yaml](ecamp3/env.yaml) that are not populated -(e.g., POSTGRES_URL). +Required fields in [env.yaml](ecamp3/env.yaml): +- NAME, DOMAIN, DOCKER_HUB_USERNAME, IMAGE_TAG +- POSTGRES_URL (and optionally POSTGRES_ADMIN_URL) +- JWT_PASSPHRASE, JWT_PRIVATE_KEY, JWT_PUBLIC_KEY +- Any enabled OAuth provider credentials (see values template) +- Optional: BACKUP/RESTORE settings, MAILER_DSN, RECAPTCHA keys.helm/ecamp3/values.yaml.gotmpl (4)
15-15: Make deployedVersion robust when git metadata is unavailableCI or local environments without a .git directory will break exec. Provide a fallback.
-deployedVersion: {{ (exec "git" (list "rev-parse" "--short" "HEAD")) | trim | quote }} +deployedVersion: {{ ((exec "git" (list "rev-parse" "--short" "HEAD")) | default "unknown") | trim | quote }}
74-74: num_threads default “false” hack can leak into runtime configBoolean false may still generate an env var (string "false"). Prefer nil and template guards or move default to app code soon as planned.
I can patch the secret/deployment template to check for presence and non-empty value so you can set num_threads: null here.
218-221: Avoid embedding basic auth credentials in values; prefer a Secret reference or htpasswd annotationStoring username/password in rendered values increases blast radius. Use a pre-created Secret or an htpasswd annotation sourced from a Secret.
I can wire this to a Secret template and have the Ingress reference it if you want.
121-121: Consider defaulting browserless.domain to a subdomain of the app domainKeeps ingress hostnames consistent out of the box.
- domain: + domain: {{ printf "browserless.%s" .Values.domain | quote }}.helm/ecamp3/helmfile.yaml (2)
6-10: Empty release name is unconventional; add a note or set a dummy nameSince this file is only used to render values (write-values), an empty name won’t hurt, but it can confuse maintainers.
-releases: - - name: "" +releases: + # Name unused: helmfile is only invoked to materialize values.yaml + - name: "values-only"
1-5: Consider inlining an explicit environment name and documenting env.yaml expectationsSmall clarity boost for folks not used to helmfile.
environments: default: values: - ./env.yaml +# Expected fields in env.yaml: NAME, DOMAIN, DOCKER_HUB_USERNAME, IMAGE_TAG, and secrets.github/actions/setup-helmfile/action.yaml (1)
1-13: Make helmfile version and platform configurable; add checksum verificationHarden supply chain and allow ARM runners without editing the action.
name: 'Setup helmfile' description: 'Sets up helmfile in /usr/local/bin/helmfile' +runs: + using: "composite" + steps: + - name: install helmfile + shell: bash + env: + HELMFILE_VERSION: ${{ inputs.version || '1.1.3' }} + run: | + set -euo pipefail + ARCH="$(uname -m)"; case "$ARCH" in x86_64) ARCH="amd64";; aarch64) ARCH="arm64";; esac + cd /tmp + url="https://github.com/helmfile/helmfile/releases/download/v${HELMFILE_VERSION}/helmfile_${HELMFILE_VERSION}_linux_${ARCH}.tar.gz" + curl -fsSL "$url" -o helmfile.tgz + tar -xzf helmfile.tgz + sudo install -m 0755 helmfile /usr/local/bin/helmfile + helmfile --version - runs: - using: "composite" - steps: - - name: install helmfile - run: | - curl -L https://github.com/helmfile/helmfile/releases/download/v1.1.3/helmfile_1.1.3_linux_amd64.tar.gz -o helmfile.tar.gz - tar -xvf /tmp/helmfile.tar.gz - mv helmfile /usr/local/bin - chmod +x /usr/local/bin/helmfile - working-directory: /tmp - shell: bash + +inputs: + version: + description: 'Helmfile version (without v prefix), e.g. 1.1.3' + required: false.helm/ecamp3/deploy.sh (3)
19-24: Use atomic Helm upgrades and wait for rolloutReduces partial-rollout failures and surfaces errors deterministically in CI.
- helm upgrade --install ecamp3-$NAME \ - $SCRIPT_DIR \ - --values $SCRIPT_DIR/values.yaml \ - --timeout "$HELM_TIMEOUT" + helm upgrade --install "ecamp3-$NAME" \ + "$SCRIPT_DIR" \ + --values "$SCRIPT_DIR/values.yaml" \ + --atomic --wait \ + --timeout "$HELM_TIMEOUT"
8-11: Improve error message for missing NAMETiny UX boost to point to the source of NAME.
if [ -z "${NAME:-}" ]; then - echo "Please specify the NAME" + echo "Please specify NAME (set it in .helm/ecamp3/env.yaml or export NAME in the environment)" exit 1 fi
5-7: REPO_DIR is unusedDrop it to satisfy static analysis and avoid confusion.
-SCRIPT_DIR=$(realpath "$(dirname "$0")") -REPO_DIR=$(realpath "$SCRIPT_DIR/../..") +SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd -P)".github/workflows/restore-backup-dev-pr.yml (1)
55-55: Prefer inputs.action over github.event.inputs.action for consistency.
In workflow_dispatch contexts, theinputscontext is simpler and avoids the longergithub.event.inputspath used elsewhere. Functionally identical here.Apply this diff:
- action: ${{ github.event.inputs.action }} + action: ${{ inputs.action }}.helm/deploy-to-cluster.sh (2)
6-6: Validate ACTION and fail fast on invalid values.
Currently any string is accepted and forwarded. Guard to accepted values ("diff" | "deploy") to catch typos early.Apply this diff:
ACTION="${1:-diff}" +case "$ACTION" in + diff|deploy) ;; + *) + echo "ERROR: ACTION must be 'diff' or 'deploy' (got: '$ACTION')" >&2 + exit 2 + ;; +esac
1-10: Harden the script prologue and quoting for portability.
- Use
set -euto fail on unset variables and errors (portable to /bin/sh).- Quote variable expansions and paths.
- Consider a fallback if
realpathis unavailable on some environments.Apply this diff:
-#!/bin/sh +#!/bin/sh +set -eu @@ -SCRIPT_DIR=$(realpath "$(dirname "$0")") -REPO_DIR=$(realpath "$SCRIPT_DIR"/..) +# Resolve SCRIPT_DIR and REPO_DIR with realpath if available, else use readlink -f. +_dir="$(dirname "$0")" +if command -v realpath >/dev/null 2>&1; then + SCRIPT_DIR="$(realpath "$_dir")" + REPO_DIR="$(realpath "$SCRIPT_DIR/..")" +else + # shellcheck disable=SC2018,SC2019 + SCRIPT_DIR="$(cd "$_dir" && pwd)" + REPO_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" +fi @@ -"$REPO_DIR/.helm/ecamp3/deploy.sh" "${ACTION}" +"$REPO_DIR/.helm/ecamp3/deploy.sh" "$ACTION".github/workflows/deployment-stage-prod.yml (1)
36-41: Make the push vs. dispatch default explicit and robust.
github.event.inputsis only defined for workflow_dispatch. While|| 'deploy'typically works, being explicit avoids edge-case surprises and improves readability.Apply this diff:
- action: ${{ github.event.inputs.action || 'deploy' }} + action: ${{ github.event_name == 'workflow_dispatch' && inputs.action || 'deploy' }}.github/workflows/restore-backup-stage-prod.yml (1)
57-62: Prefer inputs.action for workflow_dispatch and add a default to be safe.
Keeps the pattern consistent with other workflows and guards against accidental empty values.Apply this diff:
- env: ${{ github.ref_name }} - action: ${{ github.event.inputs.action }} + env: ${{ github.ref_name }} + action: ${{ inputs.action || 'diff' }}.github/workflows/deployment-devel.yml (2)
8-16: Clarify required vs default for workflow_dispatch input.
required: truewithdefault: diffis redundant. If you keep a default, setrequired: falseto avoid confusion for maintainers.- action: - description: "Choose action" - type: choice - required: true - default: diff + action: + description: "Choose action" + type: choice + required: false + default: diff
37-42: Push-trigger default is “deploy”; manual default is “diff” — confirm intention.For push events,
github.event.inputs.actionis empty, so the fallback becomesdeploy, while manual runs default todiff. If that’s intentional, add a short comment; if not, align the fallback with “diff” to prevent unintended deployments on push.Option A (keep deploy on push, document it):
env: dev - action: ${{ github.event.inputs.action || 'deploy' }} + # On pushes: default to 'deploy'. On manual runs: user input (default 'diff'). + action: ${{ github.event.inputs.action || 'deploy' }}Option B (safer default to diff everywhere):
- action: ${{ github.event.inputs.action || 'deploy' }} + action: ${{ github.event.inputs.action || 'diff' }}.github/workflows/reusable-ecamp3-deployment.yml (4)
46-53: Hardcoded job_name is brittle across callers.
job_name: Upgrade or install deployment / Deploy to Kubernetesties this reusable workflow to a specific caller job name. If another workflow renames the job, the link breaks.
- Prefer
${{ github.job }}when supported by the action, or avoidjob_nameto let it auto-detect.- Alternatively, pass
job-nameas an input from the caller and default it here.
54-61: Prefer GITHUB_TOKEN with least-privilege permissions over a PAT.
REPO_ACCESS_TOKEN(a PAT) may be unnecessary.bobheadxi/deploymentssupportsGITHUB_TOKEN. Tighten permissions with apermissions:block.jobs: deploy-ecamp3: + permissions: + contents: read + deployments: write + pull-requests: write + id-token: write @@ - - name: Create a pending GitHub deployment + - name: Create a pending GitHub deployment uses: bobheadxi/[email protected] id: deployment with: step: start - token: ${{ secrets.REPO_ACCESS_TOKEN }} + token: ${{ secrets.GITHUB_TOKEN }} env: ${{ inputs.env }}Also change the “Finish the GitHub deployment” step’s token accordingly.
113-126: Optional: gate diff/deploy steps explicitly and add concurrency by environment.Small polish: run “diff” only when requested, and prevent overlapping deploys per env/name.
deploy-ecamp3: + concurrency: + group: ecamp3-${{ inputs.env }}-${{ inputs.name || 'default' }} + cancel-in-progress: false @@ - - name: Diff deployment - run: | - ./.helm/ecamp3/deploy.sh diff || true + - name: Diff deployment + if: ${{ inputs.action == 'diff' || inputs.action == 'deploy' }} + run: ./.helm/ecamp3/deploy.sh diff || true
138-164: Posting test credentials to PRs — verify repository visibility and env.The comment includes
[email protected] / test. Ensure this is acceptable in the repo’s visibility and that feature-branch envs are isolated and disposable..helm/build-images.sh (3)
1-1: Use stricter shell flags for robustness.Switching to
/bin/shis fine. Add-ufor undefined variables.pipefailisn’t POSIX, so skip it.-#!/bin/sh +#!/bin/sh +set -eu
12-16: Include Sentry build-args only when set to avoid noisy empty args.Keeps builds clean when Sentry is intentionally omitted.
-sentry_build_args="--build-arg SENTRY_AUTH_TOKEN=$SENTRY_AUTH_TOKEN --build-arg SENTRY_ORG=$SENTRY_ORG" -sentry_build_args="$sentry_build_args --build-arg SENTRY_RELEASE_NAME=$SENTRY_RELEASE_NAME" + sba="" + [ -n "${SENTRY_AUTH_TOKEN:-}" ] && sba="$sba --build-arg SENTRY_AUTH_TOKEN=$SENTRY_AUTH_TOKEN" + [ -n "${SENTRY_ORG:-}" ] && sba="$sba --build-arg SENTRY_ORG=$SENTRY_ORG" + [ -n "${SENTRY_RELEASE_NAME:-}" ] && sba="$sba --build-arg SENTRY_RELEASE_NAME=$SENTRY_RELEASE_NAME" @@ -frontend_sentry_build_args="$sentry_build_args --build-arg SENTRY_FRONTEND_PROJECT=$SENTRY_FRONTEND_PROJECT" +frontend_sentry_build_args="$sba" +[ -n "${SENTRY_FRONTEND_PROJECT:-}" ] && frontend_sentry_build_args="$frontend_sentry_build_args --build-arg SENTRY_FRONTEND_PROJECT=$SENTRY_FRONTEND_PROJECT" @@ -print_sentry_build_args="$sentry_build_args --build-arg SENTRY_PRINT_PROJECT=$SENTRY_PRINT_PROJECT" +print_sentry_build_args="$sba" +[ -n "${SENTRY_PRINT_PROJECT:-}" ] && print_sentry_build_args="$print_sentry_build_args --build-arg SENTRY_PRINT_PROJECT=$SENTRY_PRINT_PROJECT"
35-39: Exports won’t persist beyond this script; use GITHUB_ENV if needed.
export REPO_OWNER/VERSIONonly affect the current process. If later steps need them, write to$GITHUB_ENV.-export REPO_OWNER=${docker_hub_account} -export VERSION=${version} +if [ -n "${GITHUB_ENV:-}" ]; then + { + echo "REPO_OWNER=$docker_hub_account" + echo "VERSION=$version" + } >> "$GITHUB_ENV" +fiIf no consumers exist, remove these exports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (22)
.github/actions/setup-helmfile/action.yaml(1 hunks).github/workflows/deployment-devel.yml(2 hunks).github/workflows/deployment-pr.yml(2 hunks).github/workflows/deployment-stage-prod.yml(2 hunks).github/workflows/restore-backup-dev-pr.yml(2 hunks).github/workflows/restore-backup-stage-prod.yml(2 hunks).github/workflows/reusable-dev-deployment.yml(0 hunks).github/workflows/reusable-ecamp3-deployment.yml(1 hunks).github/workflows/reusable-stage-prod-deployment.yml(0 hunks).helm/.env-example(0 hunks).helm/README.md(1 hunks).helm/build-images.sh(1 hunks).helm/deploy-to-cluster.sh(1 hunks).helm/ecamp3/.gitignore(1 hunks).helm/ecamp3/.helmignore(1 hunks).helm/ecamp3/deploy.sh(1 hunks).helm/ecamp3/env.example.yaml(1 hunks).helm/ecamp3/helmfile.yaml(1 hunks).helm/ecamp3/templates/frontend_configmap.yaml(1 hunks).helm/ecamp3/values.yaml(0 hunks).helm/ecamp3/values.yaml.gotmpl(1 hunks).helm/generate-jwt-values.sh(1 hunks)
💤 Files with no reviewable changes (4)
- .helm/ecamp3/values.yaml
- .helm/.env-example
- .github/workflows/reusable-dev-deployment.yml
- .github/workflows/reusable-stage-prod-deployment.yml
🧰 Additional context used
🪛 LanguageTool
.helm/README.md
[grammar] ~9-~9: There might be a mistake here.
Context: ...o have the following dependencies: - jq - kubectl (with a kubeconfig for the clust...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...g for the cluster you want to deploy to) - helm - helmfile - docker (with a public ...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...he cluster you want to deploy to) - helm - helmfile - docker (with a public reposit...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...you want to deploy to) - helm - helmfile - docker (with a public repository to push...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...h a public repository to push images to) - openssl ## Setup If you don't have JWT...
(QB_NEW_EN)
[grammar] ~18-~18: There might be a mistake here.
Context: ...s to) - openssl ## Setup If you don't have JWT Passphrase, public and private key ...
(QB_NEW_EN)
[grammar] ~26-~26: There might be a mistake here.
Context: ...env.yaml which are not set to any value. (e.g. POSTGRES_URL). ## Build images `...
(QB_NEW_EN)
🪛 Shellcheck (0.10.0)
.helm/ecamp3/deploy.sh
[warning] 6-6: REPO_DIR appears unused. Verify use (or export if used externally).
(SC2034)
.helm/deploy-to-cluster.sh
[warning] 8-8: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 8-8: Quote this to prevent word splitting.
(SC2046)
.helm/generate-jwt-values.sh
[warning] 7-7: REPO_DIR appears unused. Verify use (or export if used externally).
(SC2034)
🪛 actionlint (1.7.7)
.github/workflows/reusable-ecamp3-deployment.yml
32-32: input "action" of workflow_call event has the default value "diff", but it is also required. if an input is marked as required, its default value will never be used
(events)
🔇 Additional comments (13)
.helm/ecamp3/env.example.yaml (1)
1-39: Helm templates already guard againstnullforAPI_NUM_THREADSThe
values.yaml.gotmplsetsapi: num_threads: {{ .Environment.Values | getOrNil "API_NUM_THREADS" | default false }}so when
API_NUM_THREADSisnull(or unset) it becomes the booleanfalse.Then in
templates/api_configmap.yaml:{{- if .Values.api.num_threads }} NUM_THREADS: {{ .Values.api.num_threads | quote }} {{- end }}Since
falseis falsy, theNUM_THREADSkey is skipped entirely—no empty or invalid entry is emitted.No changes are needed here.
.helm/generate-jwt-values.sh (2)
35-35: Rely on trap for cleanupSince a trap is added, the explicit rm -rf is redundant but harmless. You can drop it or leave it.
7-7: SC2034: REPO_DIR unusedStatic analysis correctly flags REPO_DIR as unused. Removing it per earlier suggestion resolves the warning.
.helm/ecamp3/values.yaml.gotmpl (3)
51-52: Sentry DSN should be optional and conditionally rendered in templatesYou’ve allowed empty DSNs. Ensure templates don’t set SENTRY_ENVIRONMENT or enable SDK when DSN is empty to prevent noisy startup errors.
Would you like me to add a guard in the ConfigMap template (e.g., only render SENTRY_ENVIRONMENT when sentryDsn is non-empty)?
138-156: Nice touch: auto-enable dummy mail when MAILER_DSN is absentThis is a sensible default for dev/test environments.
13-14: Unable to verifyexecavailability in CI runnerI attempted to run the provided script but received:
helmfile: command not foundwhich means I couldn’t confirm whether Helmfile’s
exec(orwrite-values) function is available in your CI environment.• Please confirm in your CI that:
helmfile --versionruns successfully (ideally v1.1.3 or later)- the
exectemplate function is available, or- the
write-valuesfeature exists if you intended to use that insteadIf either isn’t present, consider refactoring to pass the timestamp and domain via environment variables or workflow inputs rather than relying on
exec..helm/ecamp3/deploy.sh (2)
13-16: Guard against missing env.yaml and dirty values.yamlBefore writing values, ensure env.yaml exists and values.yaml is ignored in VCS.
- Check that .helm/ecamp3/env.yaml exists or instruct the caller to run generate-jwt-values.sh.
- Verify .helm/ecamp3/.gitignore contains
values.yamlto avoid committing secrets.
15-16: Ensure Helmfile is installed and supportswrite-values
I attempted to verify on the runner, but thehelmfilebinary wasn’t found, so we can’t confirm that v1.1.3 supports thewrite-valuescommand (and your templating functions) without first installing it.Please add or update your CI to:
- Install Helmfile v1.1.3 (for example, by downloading the binary or using a package manager).
- Run the following checks before deploying:
helmfile --version helmfile write-values --help- If v1.1.3 does not support
write-values, either:
- Replace with
helmfile template; or- Move value-generation logic into your workflow scripts.
.github/workflows/restore-backup-dev-pr.yml (2)
24-31: Input wiring for "action" looks good and aligns with the new reusable workflow.
No issues—clear UI, sensible default, and constrained options.
47-47: Switch to reusable-ecamp3-deployment.yml is correct.
This unifies the callsite with the rest of the repo and reduces duplication..github/workflows/deployment-stage-prod.yml (1)
12-21: Action input UX is clear; ensure behavior is documented.
UI default is "diff", but on push you fall back to "deploy" (see Line 40). Call this out in the workflow description/readme to avoid surprises.Would you like me to add a short note to the workflow description or repo docs explaining: “Manual runs default to diff; push events default to deploy”?
.github/workflows/restore-backup-stage-prod.yml (1)
21-29: Good addition: action input matches the new reusable’s contract.
No functional concerns..github/workflows/deployment-pr.yml (1)
6-16: Adding a manual trigger is helpful for maintainers.
The input definition mirrors other workflows—consistent and clear.
That it is easier to use github action secrets and vars in the deployment. Leave dev and stage-prod in separate files that the review is somehow easier. They will be put into one workflow in the next commit. The default false for the num_threads is currently a hack because the if statement in the secret should be an if empty, not just if. There we should move the default into the application code, and then we can simplify the null handling. Also add api.dataMigrationsDir as an explicit value. This was implicit before. There we should also move the default to the application code later. I leave these improvements out that this PR remains reviewable. It's also best to diff the deleted values.yaml and the values.yaml.gotmpl. Sadly the similarity was not enough for git to mark this as rename.
…oyment.yml to reusable-ecamp3-deployment.yml
BacLuc
force-pushed
the
rewrite-deploy-ecamp3
branch
from
f7773e0 to
fafc261
Compare
Don't add it to the workflows yet because ecamp#8025 will make this a lot easier. Has merge conflict with ecamp#8025 The ttl will be made shorter ones all users have refresh tokens.
| default: diff | ||
|
|
||
| env: | ||
| NAME: ${{ inputs.name }} |
There was a problem hiding this comment.
Why are these needed in the workflow-global env? I see you using inputs.name later in the workflow
| token: ${{ secrets.REPO_ACCESS_TOKEN }} | ||
| status: ${{ job.status }} | ||
| deployment_id: ${{ steps.deployment.outputs.deployment_id }} | ||
| env_url: ${{ (inputs.env == 'staging' || inputs.env == 'prod') && format('https://{0}.{1}', vars.SUBDOMAIN, vars.DOMAIN) || format('https://{0}.{1}', inputs.name, vars.DOMAIN) }} |
There was a problem hiding this comment.
So for dev, inputs.name needs to be set, but not for staging and prod?
There was a problem hiding this comment.
if (inputs.env == 'staging' || inputs.env == 'prod')
format('https://{0}.{1}', vars.SUBDOMAIN, vars.DOMAIN)
else
format('https://{0}.{1}', inputs.name, vars.DOMAIN) }}
this is the same as before, but now in one beautiful statement.
Don't add it to the workflows yet because ecamp#8025 will make this a lot easier. Has merge conflict with ecamp#8025 The ttl will be made shorter ones all users have refresh tokens.
Don't add it to the workflows yet because ecamp#8025 will make this a lot easier. Has merge conflict with ecamp#8025 The ttl will be made shorter ones all users have refresh tokens.
helm: allow deploying the frontend without sentry dsn in frontend_configmap.yaml
Else the sentry environment is not set which gets.
This was never tested when we added this feature.
chore: rewrite ecamp3 deployment to helmfile
That it is easier to use github action secrets and vars in the deployment.
Leave dev and stage-prod in separate files that the review is somehow easier.
They will be put into one workflow in the next commit.
The default false for the num_threads is currently a hack because
the if statement in the secret should be an if empty, not just if.
There we should move the default into the application code, and
then we can simplify the null handling.
Also add api.dataMigrationsDir as an explicit value.
This was implicit before.
There we should also move the default to the application code later.
I leave these improvements out that this PR remains reviewable.
It's also best to diff the deleted values.yaml and the values.yaml.gotmpl.
Sadly the similarity was not enough for git to mark this as rename.
chore: merge reusable-dev-deployment.yml and reusable-stage-prod-deployment.yml to reusable-ecamp3-deployment.yml
Result can be seen here: #8024
I will later reduce the number of workflows, the number of entrypoint workflows is currently still the same.