Develop #70
Develop #70
Conversation
…ation Feature/helm dispatch integration
WalkthroughThe pull request introduces a comprehensive Helm dispatch capability to the CI/CD pipeline. It adds seven new inputs to the reusable build workflow (enable_helm_dispatch, helm_repository, helm_chart, helm_target_ref, helm_components_base_path, helm_env_file, helm_detect_env_changes) and a conditional dispatch-helm job that runs after successful builds. Two new reusable workflows are introduced: dispatch-helm.yml handles environment variable detection and constructs a payload for remote Helm repository updates, while helm-update-chart.yml automates the actual chart updates by modifying values.yaml, appVersion, and READMEs, then creates pull requests with Slack notifications. Sequence DiagramsequenceDiagram
participant Build as build.yml
participant DispatchHelm as dispatch-helm.yml
participant HelmRepo as Target Helm Repository
participant HelmUpdate as helm-update-chart.yml
Build->>DispatchHelm: Trigger workflow_dispatch<br/>(chart, components, version)
activate DispatchHelm
DispatchHelm->>DispatchHelm: Checkout code<br/>Detect env changes<br/>Build components payload
DispatchHelm->>HelmRepo: Dispatch workflow_dispatch<br/>(chart, components, env_vars)
deactivate DispatchHelm
activate HelmRepo
HelmRepo->>HelmUpdate: Trigger helm-update-chart.yml<br/>(payload)
deactivate HelmRepo
activate HelmUpdate
HelmUpdate->>HelmUpdate: Checkout target branch<br/>Update values.yaml<br/>Update Chart.yaml appVersion<br/>Update README matrix
HelmUpdate->>HelmUpdate: Commit & push feature branch<br/>Create PR
HelmUpdate->>HelmUpdate: Send Slack notification
deactivate HelmUpdate
Possibly related PRs
Suggested labels
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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: 0
🧹 Nitpick comments (6)
.github/workflows/helm-update-chart.yml (3)
108-149: Payload extraction is sound; minor debug artifact consideration.The use of an environment variable (
PAYLOAD_INPUT) to write the payload avoids shell quoting issues—good approach. However, lines 117-122 include hex dump debugging which could expose sensitive data in logs if the payload contains secrets.Consider either:
- Removing the hex dump in production
- Gating it behind a debug input flag
228-233:escape_sedmay not handle all special characters.The function escapes
\,&, and/, but doesn't handle$which could cause issues if values contain shell variable references. Additionally, newlines are stripped, which may truncate multi-line values.🔎 Consider a more robust escaping approach
escape_sed() { local str="$1" - # Escape: \ & / and newlines - printf '%s' "$str" | sed -e 's/[\/&\\]/\\&/g' | tr -d '\n' + # Escape: \ & / $ and handle newlines + printf '%s' "$str" | sed -e 's/[\/&\\$]/\\&/g' | tr '\n' ' ' }
295-320: Subshell issue: variables modified insidewhileloop won't persist.The pipeline
echo "$COMP_ENV_VARS" | ... | while ... doneruns thewhileloop in a subshell. Any variable modifications (like tracking added vars) won't persist after the loop exits. Currently this doesn't break functionality since no such tracking is done, but it's a gotcha if future modifications attempt to accumulate state.If you ever need to track state across iterations, consider using process substitution:
while IFS='=' read -r key value; do # ... done < <(echo "$COMP_ENV_VARS" | jq -r 'to_entries[] | "\(.key)=\(.value)"').github/workflows/build.yml (1)
303-304: DocumentHELM_REPO_TOKENsecret requirement.The
HELM_REPO_TOKENsecret is passed todispatch-helm.ymlbut isn't documented in the workflow inputs/secrets section. Callers need to know this secret is required whenenable_helm_dispatch: true.Consider adding a comment near the Helm dispatch inputs documenting this requirement.
.github/workflows/dispatch-helm.yml (2)
236-247: Manual JSON construction is fragile; consider usingjq.The component object is built via string concatenation:
COMP_OBJ="{\"name\":\"${COMP_NAME}\",\"version\":\"${VERSION}\",\"env_vars\":${ENV_VARS_JSON}}"If
COMP_NAMEorVERSIONcontain quotes or special characters, this produces invalid JSON. While these values likely come from controlled sources (git tags, mappings), usingjqwould be safer.🔎 Safer JSON construction with jq
- # Build component object - COMP_OBJ="{\"name\":\"${COMP_NAME}\",\"version\":\"${VERSION}\",\"env_vars\":${ENV_VARS_JSON}}" + # Build component object safely with jq + COMP_OBJ=$(jq -n \ + --arg name "$COMP_NAME" \ + --arg version "$VERSION" \ + --argjson env_vars "$ENV_VARS_JSON" \ + '{name: $name, version: $version, env_vars: $env_vars}')
218-230: Env var JSON construction could be simplified withjq.The manual JSON construction for
ENV_VARS_JSONis harder to maintain than usingjq. The current escaping approach (line 222) handles quotes but may struggle with backslashes or other special characters.🔎 Alternative using jq for safer JSON construction
# Build env vars JSON incrementally with jq ENV_VARS_JSON="{}" for var in $NEW_VARS; do DEFAULT_VALUE=$(grep "^${var}=" "$FULL_ENV_FILE" | cut -d'=' -f2- | sed 's/^"//;s/"$//') ENV_VARS_JSON=$(echo "$ENV_VARS_JSON" | jq --arg k "$var" --arg v "$DEFAULT_VALUE" '. + {($k): $v}') done
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build.yml.github/workflows/dispatch-helm.yml.github/workflows/helm-update-chart.yml
🔇 Additional comments (10)
.github/workflows/helm-update-chart.yml (4)
308-316: Key names are not escaped insedpatterns.While values are passed through
escape_sed, the$keyvariable is used directly in the sed pattern. If a key contains special regex characters (unlikely but possible), this could cause unexpected behavior or injection.Consider escaping the key as well, or validating that keys match expected patterns (alphanumeric + underscore):
if [[ ! "$key" =~ ^[A-Z_][A-Z0-9_]*$ ]]; then echo "::warning::Skipping invalid env var key: $key" continue fi
580-591: LGTM on Slack notification error handling.The Slack notification step properly checks the API response for success and emits warnings on failure without failing the workflow. This is a reasonable pattern for non-critical notifications.
397-458: LGTM on PR creation approach.Writing the PR body to a file (
/tmp/pr_body.md) and using--body-fileis a good practice that avoids shell escaping issues with complex markdown content.
157-177: LGTM on GPG signing configuration.The conditional GPG signing setup with a non-GPG fallback is well structured. The
if: ${{ inputs.gpg_sign_commits }}andif: ${{ !inputs.gpg_sign_commits }}conditions are mutually exclusive and clear..github/workflows/build.yml (2)
62-90: LGTM on Helm dispatch input definitions.The new inputs for Helm dispatch are well-documented with clear descriptions and sensible defaults. The
enable_helm_dispatchtoggle allows opt-in behavior, which is a good pattern for backward compatibility.
287-304: Verifycomponents_jsonformat compatibility.The
dispatch-helmjob passes${{ needs.prepare.outputs.matrix }}ascomponents_json. This matrix has format[{"name": "...", "working_dir": "..."}](from the prepare step).The
dispatch-helm.ymlworkflow normalizes this on line 159, convertingworking_dirtopath. This appears intentional and compatible, but ensure the field mapping is tested..github/workflows/dispatch-helm.yml (4)
110-125: LGTM on tag-based BEFORE_SHA detection.The logic to find the previous tag when
github.event.beforeis all zeros (as happens with tag pushes) is well-implemented. Usinggit tag --sort=-v:refnamewithgrep -A1correctly finds the previous version for comparison.
198-222: Env var pattern only matches uppercase variables.The regex
^[A-Z_][A-Z0-9_]*=only matches uppercase environment variable names. If your.env.examplefiles use lowercase or mixed-case variables, they won't be detected.This may be intentional (enforcing uppercase convention), but worth confirming.
285-300: LGTM on workflow dispatch implementation.The double JSON encoding (
jq -c .thenjq -R .) is correct—workflow_dispatch inputs must be strings, so the payload needs to be stringified. The HTTP 204 check and error handling are appropriate.
132-161: LGTM on dual-mode component input handling.The workflow cleanly supports both path-mapping mode and direct
components_jsonmode. The normalization on line 159 that convertsworking_dirtopathensures compatibility with the build workflow's matrix format.
Description
Type of Change
feat: New feature or workflowfix: Bug fixdocs: Documentation updaterefactor: Code refactoringperf: Performance improvementtest: Adding or updating testsci: CI/CD configuration changeschore: Maintenance tasksBREAKING CHANGE: Breaking change (requires major version bump)Affected Workflows
Changes Made
Breaking Changes
None / Describe breaking changes here
Testing
Checklist
Related Issues
Closes #
Related to #
Additional Notes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.