Harden, generalize, and document the helm-generic deploy action#100
Merged
Conversation
… drop dead ingress logic The helm-generic composite action is used inside the generic-chart-helm (ingress-nginx) and generic-gateway-helm-template (Gateway API) reusable workflows. Three fixes, all verified behavior-preserving for existing callers: #1 Migration Job was hardcoded to one app (`dotnet SW.Shanap.Web.dll --migrate`, `connection-string`, `ConnectionStrings__ShanapDb`), so any other app (e.g. mealivery) running the "generic" workflow executed the wrong entrypoint. Added init_job_command / init_job_secret_key / init_job_connstring_env / init_job_environment_env inputs (action + both workflows) that coalesce to the historical defaults when empty -> identical Job for existing callers, correct entrypoint for new ones. #2 The action wrote CHECKPOINT_1_STATUS/CHECKPOINT_2_STATUS into $GITHUB_ENV, colliding with the same names owned by the calling workflows (different meanings). Renamed to HELM_GENERIC_KUBECONFIG_STATUS / _DEPLOY_STATUS so the workflows' checkpoint summaries are no longer clobbered. #3 The action always injected `--set ingress.paths[0]=/`. Removed it: the ingress chart (s9genericchart) defaults ingress.paths to ["/"], and the gateway chart (s9genericchart-v2) has no ingress template at all, so removal renders identical manifests in both workflows (verified via helm template). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…efore migration #7 cleanups: - Remove dead inputs `registry_profile` and `version` (declared but never used; neither calling workflow passes them, so removal is contract-safe). - Remove the redundant `app.name` set from both workflows' extra_set_values; the action already sets it canonically from the required app_name input (identical value, verified via helm template). app.version is left in place since the workflows intentionally override the action's branch-based value with the semver. - Fix README.md and AGENTS.md, which incorrectly described helm-generic as a "checkout + lint + package + push" CI action; it is a deployer. Namespace ordering fix: - The migration Job ran before the Helm step's --create-namespace, so a first deploy to a fresh namespace failed at the Job apply. Ensure the namespace exists first via the idempotent `kubectl create --dry-run=client | apply` pattern already used by generic-gateway-helm-template. No-op when the namespace already exists. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…pin kubectl #5 kubeconfig handling: - Write the decoded kubeconfig to $RUNNER_TEMP instead of the workspace, so the credential is no longer left in the checked-out tree, and add an `if: always()` cleanup step that removes it (matters on self-hosted/reused runners). - Adopt the generic-gateway-helm-template workflow's decoder (base64 with raw fallback) and add a fail-fast apiVersion validation. Behavior-preserving for the two real input forms (base64 secret, raw YAML) — both decode to a semantically identical kubeconfig — and now rejects malformed input with a clear message instead of letting Helm fail later. #6: - Remove `actions/checkout@v6`: the deploy job uses no repo files (chart comes from chart_repo, image from the registry); the only workspace reference was the old kubeconfig path, now in RUNNER_TEMP. Updated the chart/chart_repo input docs to note local chart paths are not supported. - Pin kubectl to v1.34.0 instead of 'latest' for reproducibility (only basic core-resource ops are used, so cluster skew is not a concern; documented as bumpable to track the cluster's minor version). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… + robustness
Addresses the cold-review findings M1-M4 and m1-m5 (SHA-pinning intentionally
deferred):
M1 - Rewrite stale input docs: kubeconfig_data no longer references the removed
<PROFILE> secret concept; init_job_image no longer hardcodes "--migrate"/EF
Core/doadmin language and now points at init_job_command.
M2 - Reword the leftover "BASE_SET_ARGS now only…/modified input description" comment.
M3 - Bind ${{ inputs.* }} to env: in the Announce, kubeconfig-notice, Set KUBECONFIG
and Report-failure steps (GitHub injection-hardening guidance); clears shellcheck
SC2296 noise. Behavior-identical.
M4 - Assemble extra_set_values and extra_args as bash arrays instead of unquoted
word-split strings, so --set values containing spaces stay intact. Token stream
is identical for current (space-free) callers.
m1 - Add `atomic` input (default 'true') -> pass --atomic so a failed deploy rolls
back automatically. (Behavior change: failed upgrades now roll back.)
m2 - Migration wait now polls for Complete/Failed and surfaces a failed Job
immediately instead of blocking the full 5m timeout.
m3 - Show-status pod selector tries app.kubernetes.io/instance=<release> (gateway
chart) then falls back to app=<app.name> (ingress chart), so it matches both.
m4 - Rename action to 'Helm Generic Deploy' and add author: Simplify9.
m5 - generic-gateway auto-onboard step now traps EXIT to remove its decoded
kubeconfig from RUNNER_TEMP.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…kflow input
F1 - Guard the helm-upgrade array expansions with ${arr[@]+"${arr[@]}"} so an empty
EXTRA_SET_ARGS / ATOMIC_ARGS / EXTRA_ARGS_ARR no longer triggers "unbound variable"
under `set -u` on bash < 4.4 (a latent regression from the earlier array refactor).
Verified on bash 3.2 for both empty and populated arrays; no effect on the two
consumers (they always pass non-empty arrays on ubuntu-latest / bash 5.2).
F2 - Add an `atomic` input (default 'true') to both reusable workflows and pass it
through to the action, so workflow callers can opt out of --atomic auto-rollback.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
N1 - `helm repo add s9generic` previously errored when the alias already existed with
a different URL (reused/self-hosted runner); the `|| true` swallowed it and the
deploy then pulled from the stale URL. Use --force-update so the alias is repointed
at the current chart_repo. Also drop `2>/dev/null || true` so a genuine repo-add
failure (e.g. unreachable repo) fails fast with a clear error instead of proceeding
to a doomed chart ref, and scope `helm repo update` to the added repo.
Verified with helm: same-alias/new-URL now repoints correctly; scoped update works on
Helm 4. No effect on ephemeral GitHub-hosted runners.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Combine the three CHECKPOINT_*_STATUS echoes in the ci job's announce step into a
single `{ ... } >> "$GITHUB_ENV"` block. Behavior identical; clears the only actionlint
finding in this workflow. Independent of the helm-generic changes (sw-cicd uses
helm-deploy, not helm-generic).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
MusaMisto
added
bug
|
Warning Review limit reached
More reviews will be available in 50 minutes and 33 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe ChangesHelm Generic Deploy Overhaul
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/generic-gateway-helm-template.yml (1)
724-737:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden kubeconfig file permissions before use
Line 729/Line 731 writes kubeconfig material, but there is no
chmod 600before exportingKUBECONFIG. On reused/self-hosted runners, permissive umask can expose credentials to other local users/processes.Suggested fix
if echo "$KUBECONFIG_DATA" | grep -q 'apiVersion:'; then printf "%s" "$KUBECONFIG_DATA" > "$KCFG_PATH" else if printf "%s" "$KUBECONFIG_DATA" | base64 -d > "$KCFG_PATH" 2>/dev/null; then true else printf "%s" "$KUBECONFIG_DATA" > "$KCFG_PATH" fi fi + chmod 600 "$KCFG_PATH" export KUBECONFIG="$KCFG_PATH"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/generic-gateway-helm-template.yml around lines 724 - 737, The kubeconfig file is written to disk but its permissions are not explicitly hardened before being exported and used. On self-hosted or reused runners with permissive umask settings, this could expose sensitive credentials to other local users or processes. After writing the kubeconfig content at the printf statements (lines 729 and 731), add a chmod 600 command to restrict file permissions to the current user only, before the KUBECONFIG environment variable is exported at line 737..github/actions/helm-generic/action.yml (2)
181-208:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast when
init_job_imageis set butinit_job_secret_nameis empty.At Line 188,
INIT_JOB_SECRET_NAMEis consumed in the Job manifest (Lines 238-239) without an upfront guard. If omitted, the failure occurs later with a less actionable Kubernetes error.Suggested fix
# Coalesce to the historical defaults so callers that don't override keep identical behavior. CMD_STR="${INIT_JOB_COMMAND:-dotnet SW.Shanap.Web.dll --migrate}" SECRET_KEY="${INIT_JOB_SECRET_KEY:-connection-string}" CONNSTRING_ENV="${INIT_JOB_CONNSTRING_ENV:-ConnectionStrings__ShanapDb}" ENVIRONMENT_ENV="${INIT_JOB_ENVIRONMENT_ENV:-ASPNETCORE_ENVIRONMENT}" + + if [ -z "${INIT_JOB_SECRET_NAME:-}" ]; then + echo "init_job_secret_name is required when init_job_image is set." >&2 + exit 1 + fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/helm-generic/action.yml around lines 181 - 208, Add a validation check in the bash script section of the "Run DB migration Job (init container equivalent)" step to fail fast when INIT_JOB_IMAGE is set but INIT_JOB_SECRET_NAME is empty. Place this validation check early in the script, right after the environment variables are set (following the set -euo pipefail line and before any subsequent commands), and use a clear error message to indicate that init_job_secret_name is required when init_job_image is specified.
358-367:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a final always-run
$GITHUB_STEP_SUMMARYtable step.The final step currently only removes kubeconfig. The 4-pillar framework requires a summary table in the last
if: always()step, and summary heredocs must use a randomized delimiter.As per coding guidelines: “Every composite action must follow the 4-pillar log output framework … (4) Summarized table in
$GITHUB_STEP_SUMMARYon the last step withif: always(),” and “useEOF=$(dd if=/dev/urandom bs=15 count=1 status=none | base64)for heredoc delimiter.”Suggested fix
- name: Clean up kubeconfig if: always() shell: bash @@ # self-hosted/reused runners). No-op if the step never produced a path. [ -n "${KCFG_PATH:-}" ] && rm -f "$KCFG_PATH" || true + + - name: Summarize Helm Generic Deploy + if: always() + shell: bash + run: | + EOF=$(dd if=/dev/urandom bs=15 count=1 status=none | base64) + cat <<$EOF >> "$GITHUB_STEP_SUMMARY" + | Checkpoint | Status | + |---|---| + | Kubeconfig | ${HELM_GENERIC_KUBECONFIG_STATUS:-⏭️ Skipped} | + | Helm deploy | ${HELM_GENERIC_DEPLOY_STATUS:-⏭️ Skipped} | + $EOF🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/helm-generic/action.yml around lines 358 - 367, The "Clean up kubeconfig" step with if: always() currently only removes the kubeconfig file but is missing the required summary table output to $GITHUB_STEP_SUMMARY as mandated by the 4-pillar log output framework. Enhance this step to add a summary table section after the kubeconfig cleanup logic that writes structured output to $GITHUB_STEP_SUMMARY, using a randomized heredoc delimiter created with EOF=$(dd if=/dev/urandom bs=15 count=1 status=none | base64) to safely handle the heredoc content.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/helm-generic/action.yml:
- Around line 105-106: The helm-generic action currently only publishes
HELM_GENERIC_KUBECONFIG_STATUS and HELM_GENERIC_DEPLOY_STATUS env variables, but
the 4-pillar checkpoint convention requires also publishing corresponding
CHECKPOINT_N_STATUS env keys for consistent downstream status handling. Add echo
statements that alias each HELM_GENERIC_*_STATUS assignment to the appropriate
CHECKPOINT_N_STATUS env var (for example, when setting
HELM_GENERIC_KUBECONFIG_STATUS, also set CHECKPOINT_1_STATUS to the same value).
This change needs to be applied at all status initialization points in the
action file (lines around 105-106, 158-159, 326-327, and 356).
- Around line 173-179: The kubectl version specified in the Install kubectl step
using azure/setup-kubectl@v5 is drifting from the repository baseline. Update
the version field in the with section of this step from the current pinned
version to v1.33.0 to align with the coding guidelines that require kubectl CLI
default version to be v1.33.0.
- Around line 297-305: The helm-generic action currently processes all Helm
values, including secrets, through a single EXTRA_SET_VALUES variable using the
--set parameter, which causes secret values with special characters to parse
incorrectly and violates security guidelines. Add a new input parameter called
extra_set_secret_values to the action, then create similar script logic to the
existing EXTRA_SET_ARGS block (processing EXTRA_SET_VALUES with --set) but
process the new EXTRA_SET_SECRET_VALUES with --set-string instead. Update all
workflow files that call this action to separate secret values from config
values by moving secrets.helm-set-secret-values from extra_set_values to the new
extra_set_secret_values input parameter, aligning with how helm-deploy and
helm-deploy-s9generic already handle this separation.
In `@README.md`:
- Line 762: The README documentation contains inconsistent guidance about
kubeconfig formatting. While the helm-generic action row correctly lists
kubeconfig_data as a parameter, the nearby troubleshooting and secret guidance
sections still instruct users to base64-encode the kubeconfig, which conflicts
with the action now accepting both raw YAML and base64-encoded formats. Review
all sections in the README that discuss kubeconfig setup, secret configuration,
and troubleshooting guidance, and update them to clarify that the
kubeconfig_data parameter can accept either raw YAML content or base64-encoded
data, ensuring consistency across all documentation that references kubeconfig
handling.
---
Outside diff comments:
In @.github/actions/helm-generic/action.yml:
- Around line 181-208: Add a validation check in the bash script section of the
"Run DB migration Job (init container equivalent)" step to fail fast when
INIT_JOB_IMAGE is set but INIT_JOB_SECRET_NAME is empty. Place this validation
check early in the script, right after the environment variables are set
(following the set -euo pipefail line and before any subsequent commands), and
use a clear error message to indicate that init_job_secret_name is required when
init_job_image is specified.
- Around line 358-367: The "Clean up kubeconfig" step with if: always()
currently only removes the kubeconfig file but is missing the required summary
table output to $GITHUB_STEP_SUMMARY as mandated by the 4-pillar log output
framework. Enhance this step to add a summary table section after the kubeconfig
cleanup logic that writes structured output to $GITHUB_STEP_SUMMARY, using a
randomized heredoc delimiter created with EOF=$(dd if=/dev/urandom bs=15 count=1
status=none | base64) to safely handle the heredoc content.
In @.github/workflows/generic-gateway-helm-template.yml:
- Around line 724-737: The kubeconfig file is written to disk but its
permissions are not explicitly hardened before being exported and used. On
self-hosted or reused runners with permissive umask settings, this could expose
sensitive credentials to other local users or processes. After writing the
kubeconfig content at the printf statements (lines 729 and 731), add a chmod 600
command to restrict file permissions to the current user only, before the
KUBECONFIG environment variable is exported at line 737.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d74f933e-fd44-4e89-af5a-c35f7988d6ac
📒 Files selected for processing (6)
.github/actions/helm-generic/action.yml.github/workflows/generic-chart-helm.yml.github/workflows/generic-gateway-helm-template.yml.github/workflows/sw-cicd.ymlAGENTS.mdREADME.md
Comment on lines
+105
to
+106
| echo "HELM_GENERIC_KUBECONFIG_STATUS=⏳ Pending" >> "$GITHUB_ENV" | ||
| echo "HELM_GENERIC_DEPLOY_STATUS=⏳ Pending" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
Checkpoint env contract should still publish CHECKPOINT_1_STATUS / CHECKPOINT_2_STATUS.
These lines only write HELM_GENERIC_* statuses now. The shared 4-pillar checkpoint convention expects CHECKPOINT_N_STATUS env keys for consistent downstream status handling. Please publish both (aliasing is enough).
As per coding guidelines: “Every composite action must follow the 4-pillar log output framework … with CHECKPOINT_N_STATUS env vars.”
Suggested fix
echo "HELM_GENERIC_KUBECONFIG_STATUS=⏳ Pending" >> "$GITHUB_ENV"
echo "HELM_GENERIC_DEPLOY_STATUS=⏳ Pending" >> "$GITHUB_ENV"
+ echo "CHECKPOINT_1_STATUS=⏳ Pending" >> "$GITHUB_ENV"
+ echo "CHECKPOINT_2_STATUS=⏳ Pending" >> "$GITHUB_ENV"
@@
echo "HELM_GENERIC_KUBECONFIG_STATUS=✅ PASSED" >> "$GITHUB_ENV"
+ echo "CHECKPOINT_1_STATUS=✅ PASSED" >> "$GITHUB_ENV"
@@
echo "HELM_GENERIC_DEPLOY_STATUS=✅ PASSED" >> "$GITHUB_ENV"
+ echo "CHECKPOINT_2_STATUS=✅ PASSED" >> "$GITHUB_ENV"Also applies to: 158-159, 326-327, 356-356
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/actions/helm-generic/action.yml around lines 105 - 106, The
helm-generic action currently only publishes HELM_GENERIC_KUBECONFIG_STATUS and
HELM_GENERIC_DEPLOY_STATUS env variables, but the 4-pillar checkpoint convention
requires also publishing corresponding CHECKPOINT_N_STATUS env keys for
consistent downstream status handling. Add echo statements that alias each
HELM_GENERIC_*_STATUS assignment to the appropriate CHECKPOINT_N_STATUS env var
(for example, when setting HELM_GENERIC_KUBECONFIG_STATUS, also set
CHECKPOINT_1_STATUS to the same value). This change needs to be applied at all
status initialization points in the action file (lines around 105-106, 158-159,
326-327, and 356).
Source: Coding guidelines
Comment on lines
+173
to
+179
| - name: Install kubectl | ||
| uses: azure/setup-kubectl@v5 | ||
| with: | ||
| version: 'latest' | ||
| # Pinned for reproducibility (avoids a new kubectl silently changing CI behavior). | ||
| # Only basic core-resource ops are used here (apply/wait/get/create), so exact skew | ||
| # with the cluster is not critical; bump this to track your cluster's minor version. | ||
| version: 'v1.34.0' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify kubectl version pins under actions/workflows.
rg -n "azure/setup-kubectl@v5|version:\s*'v1\.[0-9]+\.[0-9]+'" .github/actions .github/workflowsRepository: simplify9/.github
Length of output: 459
🏁 Script executed:
sed -n '173,179p' .github/actions/helm-generic/action.ymlRepository: simplify9/.github
Length of output: 439
kubectl default version drifts from repository baseline.
Line 179 pins v1.35.0, but repo policy defines the default kubectl CLI version as v1.33.0.
As per coding guidelines: "Helm CLI default version must be v4.2.0 and kubectl CLI default version must be v1.33.0."
Suggested fix
- version: 'v1.35.0'
+ version: 'v1.33.0'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Install kubectl | |
| uses: azure/setup-kubectl@v5 | |
| with: | |
| version: 'latest' | |
| # Pinned for reproducibility (avoids a new kubectl silently changing CI behavior). | |
| # Only basic core-resource ops are used here (apply/wait/get/create), so exact skew | |
| # with the cluster is not critical; bump this to track your cluster's minor version. | |
| version: 'v1.34.0' | |
| - name: Install kubectl | |
| uses: azure/setup-kubectl@v5 | |
| with: | |
| # Pinned for reproducibility (avoids a new kubectl silently changing CI behavior). | |
| # Only basic core-resource ops are used here (apply/wait/get/create), so exact skew | |
| # with the cluster is not critical; bump this to track your cluster's minor version. | |
| version: 'v1.33.0' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/actions/helm-generic/action.yml around lines 173 - 179, The kubectl
version specified in the Install kubectl step using azure/setup-kubectl@v5 is
drifting from the repository baseline. Update the version field in the with
section of this step from the current pinned version to v1.33.0 to align with
the coding guidelines that require kubectl CLI default version to be v1.33.0.
Source: Coding guidelines
Comment on lines
+297
to
305
| # Build "--set key=value" pairs as an array so values containing spaces stay intact | ||
| # (an unquoted string would word-split them). | ||
| EXTRA_SET_ARGS=() | ||
| if [ -n "${EXTRA_SET_VALUES// /}" ]; then | ||
| while IFS= read -r line; do | ||
| [ -z "${line}" ] && continue | ||
| KV=$(echo "$line" | sed 's/^ *//;s/ *$//') | ||
| [ -z "$KV" ] && continue | ||
| EXTRA_SET_ARGS+=" --set $KV" | ||
| EXTRA_SET_ARGS+=( --set "$KV" ) | ||
| done <<< "$EXTRA_SET_VALUES" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify callers currently mix secret and non-secret Helm values in one channel.
rg -n --iglob '.github/workflows/*.yml' '\$\{\{\s*secrets\.helm-set-secret-values\s*\}\}|extra_set_values|extra-set-values'Repository: simplify9/.github
Length of output: 210
🏁 Script executed:
#!/bin/bash
# Explore .github directory structure
echo "=== Directory structure of .github ==="
find .github -type f -name "*.yml" | head -20Repository: simplify9/.github
Length of output: 973
🏁 Script executed:
#!/bin/bash
# Look for the helm-generic action and the workflow mentioned
echo "=== Checking for helm-generic action.yml ==="
find .github -name "action.yml" -path "*helm-generic*"
echo ""
echo "=== Checking for generic-gateway-helm-template workflow ==="
find .github -name "*generic-gateway-helm*" -type fRepository: simplify9/.github
Length of output: 259
🏁 Script executed:
#!/bin/bash
# List all workflows to understand the structure
echo "=== All workflow files ==="
ls -la .github/workflows/ | head -20Repository: simplify9/.github
Length of output: 1201
🏁 Script executed:
#!/bin/bash
# Inspect helm-generic action.yml - get file size and relevant sections
echo "=== Checking helm-generic/action.yml size and structure ==="
wc -l .github/actions/helm-generic/action.ymlRepository: simplify9/.github
Length of output: 165
🏁 Script executed:
#!/bin/bash
# Read helm-generic action.yml to see inputs definition and the implementation around lines 297-305
echo "=== Inputs section of helm-generic action.yml ==="
sed -n '1,100p' .github/actions/helm-generic/action.yml | grep -A 20 "inputs:"Repository: simplify9/.github
Length of output: 753
🏁 Script executed:
#!/bin/bash
# Check the specific line range 297-305 mentioned in the review
echo "=== Lines 297-305 from helm-generic action.yml ==="
sed -n '297,305p' .github/actions/helm-generic/action.yml
echo ""
echo "=== Lines 319-325 from helm-generic action.yml ==="
sed -n '319,325p' .github/actions/helm-generic/action.ymlRepository: simplify9/.github
Length of output: 951
🏁 Script executed:
#!/bin/bash
# Get the env section to see how EXTRA_SET_VALUES is set
echo "=== Looking for env section around lines 280-310 ==="
sed -n '280,310p' .github/actions/helm-generic/action.ymlRepository: simplify9/.github
Length of output: 1445
🏁 Script executed:
#!/bin/bash
# Check the full inputs section including extra_set_values description
echo "=== Full inputs section ==="
sed -n '/^inputs:/,/^runs:/p' .github/actions/helm-generic/action.ymlRepository: simplify9/.github
Length of output: 3590
🏁 Script executed:
#!/bin/bash
# Check generic-gateway-helm-template.yml around lines 905-911 where it calls helm-generic
echo "=== Lines 900-920 from generic-gateway-helm-template.yml ==="
sed -n '900,920p' .github/workflows/generic-gateway-helm-template.ymlRepository: simplify9/.github
Length of output: 1264
🏁 Script executed:
#!/bin/bash
# Search for other workflows that might use helm-generic action and secrets
echo "=== Searching for helm-generic action usage in workflows ==="
rg -n "helm-generic" .github/workflows/
echo ""
echo "=== Searching for secrets.helm-set-secret-values usage ==="
rg -n "helm-set-secret-values" .github/workflows/Repository: simplify9/.github
Length of output: 1591
🏁 Script executed:
#!/bin/bash
# Check if there are other action files that might have similar Helm issues
echo "=== Checking helm-deploy and helm-deploy-s9generic actions ==="
rg -n "helm-set-string|--set-string" .github/actions/Repository: simplify9/.github
Length of output: 820
Separate secret Helm values from regular config and use --set-string for secrets.
The helm-generic action currently processes all dynamic Helm values—including secrets—through a single --set parameter (lines 297-305). In generic-gateway-helm-template.yml (line 911), ${{ secrets.helm-set-secret-values }} is mixed into extra_set_values, causing secret values with special characters to be parsed incorrectly and violating the coding guideline: "When using Helm, never mix config and secrets in a single parameter — use helm-set-values with --set for non-sensitive config and helm-set-secret-values with --set-string for secrets."
Other Helm actions (helm-deploy, helm-deploy-s9generic) correctly separate and use --set-string for secrets. This action requires a new input (extra_set_secret_values) with corresponding script logic and workflow contract updates to align with security best practices.
Suggested implementation
inputs:
+ extra_set_secret_values:
+ description: |-
+ Optional multi-line string of Helm secret values (one key=value per line),
+ applied with --set-string.
+ required: false
+ default: ''
...
env:
APP_NAME: ${{ inputs.app_name }}
NAMESPACE: ${{ inputs.namespace }}
CHART: ${{ inputs.chart }}
CHART_REPO: ${{ inputs.chart_repo }}
HELM_TIMEOUT: ${{ inputs.helm_timeout }}
EXTRA_SET_VALUES: ${{ inputs.extra_set_values }}
+ EXTRA_SET_SECRET_VALUES: ${{ inputs.extra_set_secret_values }}
EXTRA_ARGS: ${{ inputs.extra_args }}
...
EXTRA_SET_ARGS=()
if [ -n "${EXTRA_SET_VALUES// /}" ]; then
while IFS= read -r line; do
@@
done <<< "$EXTRA_SET_VALUES"
fi
+
+ EXTRA_SET_STRING_ARGS=()
+ if [ -n "${EXTRA_SET_SECRET_VALUES// /}" ]; then
+ while IFS= read -r line; do
+ KV=$(echo "$line" | sed 's/^ *//;s/ *$//')
+ [ -z "$KV" ] && continue
+ EXTRA_SET_STRING_ARGS+=( --set-string "$KV" )
+ done <<< "$EXTRA_SET_SECRET_VALUES"
+ fi
helm upgrade "${APP_NAME}" "$CHART_REF" \
--install \
--namespace "${NAMESPACE}" \
--create-namespace \
"${BASE_SET_ARGS[@]}" ${EXTRA_SET_ARGS[@]+"${EXTRA_SET_ARGS[@]}"} \
+ ${EXTRA_SET_STRING_ARGS[@]+"${EXTRA_SET_STRING_ARGS[@]}"} \
--timeout "${HELM_TIMEOUT}" \
${ATOMIC_ARGS[@]+"${ATOMIC_ARGS[@]}"} ${EXTRA_ARGS_ARR[@]+"${EXTRA_ARGS_ARR[@]}"}Update workflows (generic-gateway-helm-template.yml line 911, generic-chart-helm.yml, and others) to move ${{ secrets.helm-set-secret-values }} from extra_set_values to the new extra_set_secret_values input.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/actions/helm-generic/action.yml around lines 297 - 305, The
helm-generic action currently processes all Helm values, including secrets,
through a single EXTRA_SET_VALUES variable using the --set parameter, which
causes secret values with special characters to parse incorrectly and violates
security guidelines. Add a new input parameter called extra_set_secret_values to
the action, then create similar script logic to the existing EXTRA_SET_ARGS
block (processing EXTRA_SET_VALUES with --set) but process the new
EXTRA_SET_SECRET_VALUES with --set-string instead. Update all workflow files
that call this action to separate secret values from config values by moving
secrets.helm-set-secret-values from extra_set_values to the new
extra_set_secret_values input parameter, aligning with how helm-deploy and
helm-deploy-s9generic already handle this separation.
Source: Coding guidelines
| | `helm-deploy` | Profile-based deploy; supports `init_job_image` for pre-deploy DB migration Jobs | `app_name`, `namespace`, `kubeconfig_data` | | ||
| | `helm-deploy-s9generic` | Deploy `s9genericchart` from `https://charts.sf9.io`; handles `set-values` (`--set`) and `set-string-values` (`--set-string`) separately | `chart-name`, `chart-version`, `kubeconfig` | | ||
| | `helm-generic` | Checkout + Helm lint + package + push in a single composite | `app-name`, `version` | | ||
| | `helm-generic` | Deploy a Helm chart (`helm upgrade --install`) with optional pre-deploy DB migration Job. Used by the `generic-chart-helm` and `generic-gateway-helm-template` reusable workflows | `app_name`, `namespace`, `kubeconfig_data` | |
There was a problem hiding this comment.
Document the kubeconfig decoding behavior too.
This row is fine, but the surrounding README still tells callers to base64-encode kubeconfig, which conflicts with the action now accepting raw YAML or base64. Please align the nearby troubleshooting/secret guidance so the docs stay consistent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 762, The README documentation contains inconsistent
guidance about kubeconfig formatting. While the helm-generic action row
correctly lists kubeconfig_data as a parameter, the nearby troubleshooting and
secret guidance sections still instruct users to base64-encode the kubeconfig,
which conflicts with the action now accepting both raw YAML and base64-encoded
formats. Review all sections in the README that discuss kubeconfig setup, secret
configuration, and troubleshooting guidance, and update them to clarify that the
kubeconfig_data parameter can accept either raw YAML content or base64-encoded
data, ensuring consistency across all documentation that references kubeconfig
handling.
omarghatasheh
approved these changes
Jun 14, 2026
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
2 participants
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
Hardens and de-app-specifies the
helm-genericcomposite deploy action and aligns its twoconsumers (
generic-chart-helm= ingress-nginx,generic-gateway-helm-template= Gateway API).Driven by a full cold review covering correctness, security, performance, docs, and best practices.
All behavior-affecting changes were verified to preserve existing behavior except the two
intentional ones called out below.
Why
The action was named/documented as "generic" but had app-specific logic hardcoded, owned state
that collided with the calling workflows, and carried several latent shell/robustness issues.
Changes
Correctness
init_job_command,init_job_secret_key,init_job_connstring_env,init_job_environment_env). Previously hardcodeddotnet SW.Shanap.Web.dll --migrate/ConnectionStrings__ShanapDb, which ran the wrongentrypoint for any other app. Empty values coalesce to the historical defaults → identical
Job for existing callers.
kubectl create --dry-run | apply) instead of failing on a first deploy to a fresh namespace.ingress.paths[0]=/injection — routing is owned by theworkflows; verified manifests render identically for both the ingress chart (defaults
["/"])and the gateway chart (no ingress template).
CHECKPOINT_1/2_STATUSinto the jobenv, clobbering the names the workflows own; renamed to
HELM_GENERIC_*.Failedinstead of blocking the full 5m timeout.Security
$RUNNER_TEMP(not the workspace),chmod 600, validated, and removedon exit; the gateway onboarding pre-step now traps EXIT to clean its copy too.
${{ inputs.* }}are bound toenv:instead of interpolated intorun:bodies(injection-hardening; also clears shellcheck SC2296).
Robustness / best practices
extra_set_values/extra_argsassembled as bash arrays (values with spaces survive), withempty-array guards safe under
set -udown to bash 3.2.helm repo add --force-update(repoints a stale alias on reused runners) and fail-fast onrepo-add errors;
helm repo updatescoped to the added repo.actions/checkout; pinned kubectl tov1.34.0; removed dead inputs(
registry_profile,version) and a duplicateapp.name --set.Documentation
README.md/AGENTS.md(the action was mislabeled "lint + package + push"; it is adeployer) and rewrote stale input descriptions.
Helm Generic Deployand addedauthor:.Consumers
init_job_*andatomicas pass-through inputs on both reusable workflows.sw-cicd.yml: grouped consecutiveGITHUB_ENVwrites (shellcheck SC2129). (Independent of theaction; uses
helm-deploy, nothelm-generic.)--atomicis now on by default (atomicinput, defaulttrue): a failedhelm upgradenow rolls back automatically. Opt out with
atomic: 'false'.previous command.
Notes for reviewers
--atomicrolls the deploy back, the migration isnot reverted (inherent to migrate-before-deploy).
--debugviaextra_argsmakes Helm print computed values (incl. secrets fromextra_set_values) to logs.Verification
actionlintclean on both workflows;bash -n+shellcheckclean on the action; migration-Jobrendering, manifest equivalence, kubeconfig decode, and helm-arg assembly each validated locally
(incl. the bash 3.2 empty-array floor)
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation