Refactor deploy action to use helm upgrade via setup_combine.py#4216
Refactor deploy action to use helm upgrade via setup_combine.py#4216imnasnainaec merged 15 commits intomasterfrom
Conversation
github-actions
Bot
added
documentation
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4216 +/- ##
=======================================
Coverage 75.94% 75.94%
=======================================
Files 303 303
Lines 11352 11352
Branches 1403 1403
=======================================
Hits 8621 8621
Misses 2330 2330
Partials 401 401
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
imnasnainaec
force-pushed
the
copilot/fix-deployment-helm-charts
branch
from
e486eeb to
f27aff0
Compare
imnasnainaec
force-pushed
the
copilot/fix-deployment-helm-charts
branch
from
8f7a479 to
e3b2c85
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe deployment infrastructure migrates from direct Changes
Sequence DiagramsequenceDiagram
participant GHA as GitHub Actions
participant PythonScript as setup_combine.py
participant Helm as Helm
participant K8s as Kubernetes
GHA->>GHA: Checkout code & install dependencies
GHA->>Helm: Install Helm (helm_version)
GHA->>K8s: Install kubectl
GHA->>PythonScript: Execute with --context, --repo, --tag, --target
PythonScript->>K8s: Set kubeconfig context
PythonScript->>K8s: Retrieve current values (non-interactive mode)
PythonScript->>Helm: helm upgrade with merged values
Helm->>K8s: Apply Helm release to cluster
K8s-->>Helm: Deployment status
Helm-->>PythonScript: Release updated
PythonScript-->>GHA: Deployment complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
imnasnainaec
force-pushed
the
copilot/fix-deployment-helm-charts
branch
3 times, most recently
from
12ce2e2 to
fd3fd1b
Compare
imnasnainaec
force-pushed
the
copilot/fix-deployment-helm-charts
branch
3 times, most recently
from
7ec37f0 to
4284414
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
deploy/scripts/setup_combine.py (2)
198-198: Consider unpacking for list construction (optional).Static analysis suggests using
[*helm_cmd, "--namespace", chart_namespace]instead of concatenation. This is a minor style preference for modern Python.♻️ Optional refactor using unpacking
- namespace_cmd = helm_cmd + ["--namespace", chart_namespace] + namespace_cmd = [*helm_cmd, "--namespace", chart_namespace]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/scripts/setup_combine.py` at line 198, The current list concatenation that builds namespace_cmd from helm_cmd and ["--namespace", chart_namespace] should be refactored to use list unpacking for clarity and modern Python style: replace the expression that assigns namespace_cmd (which uses helm_cmd + ["--namespace", chart_namespace]) with an unpacking form that expands helm_cmd into the new list and appends "--namespace" and chart_namespace; locate the assignment to namespace_cmd and update it accordingly, keeping the same values and order.
203-207: Consider unpacking for list construction (optional).Similar to line 198, static analysis suggests unpacking instead of list concatenation.
♻️ Optional refactor using unpacking
logging.info(f" Deleting chart: {chart}") - delete_cmd = namespace_cmd + ["delete", chart] + delete_cmd = [*namespace_cmd, "delete", chart] if args.dry_run: delete_cmd.append("--dry-run")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/scripts/setup_combine.py` around lines 203 - 207, Replace the list concatenation when building delete_cmd with list unpacking to match the pattern used elsewhere: build delete_cmd by unpacking namespace_cmd and appending the "delete" and chart elements (so subsequent conditional append of "--dry-run" and the run_cmd(namespace_cmd) invocation remain unchanged); update the construction near the existing namespace_cmd, delete_cmd, args.dry_run, chart, and run_cmd usage accordingly to use unpacking rather than namespace_cmd + ["delete", chart].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@deploy/scripts/setup_combine.py`:
- Line 198: The current list concatenation that builds namespace_cmd from
helm_cmd and ["--namespace", chart_namespace] should be refactored to use list
unpacking for clarity and modern Python style: replace the expression that
assigns namespace_cmd (which uses helm_cmd + ["--namespace", chart_namespace])
with an unpacking form that expands helm_cmd into the new list and appends
"--namespace" and chart_namespace; locate the assignment to namespace_cmd and
update it accordingly, keeping the same values and order.
- Around line 203-207: Replace the list concatenation when building delete_cmd
with list unpacking to match the pattern used elsewhere: build delete_cmd by
unpacking namespace_cmd and appending the "delete" and chart elements (so
subsequent conditional append of "--dry-run" and the run_cmd(namespace_cmd)
invocation remain unchanged); update the construction near the existing
namespace_cmd, delete_cmd, args.dry_run, chart, and run_cmd usage accordingly to
use unpacking rather than namespace_cmd + ["delete", chart].
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 052b3ea2-04e3-4f46-8fc2-12d4e1526a5d
📒 Files selected for processing (6)
.github/actions/combine-deploy-update/action.yml.github/workflows/deploy_qa.yml.github/workflows/deploy_release.ymldeploy/ansible/vars/k3s_versions.ymldeploy/scripts/helm_utils.pydeploy/scripts/setup_combine.py
✅ Files skipped from review due to trivial changes (1)
- deploy/ansible/vars/k3s_versions.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/actions/combine-deploy-update/action.yml
- .github/workflows/deploy_qa.yml
Co-authored-by: imnasnainaec <6411521+imnasnainaec@users.noreply.github.com> Agent-Logs-Url: https://github.com/sillsdev/TheCombine/sessions/4238e849-5ac8-431c-a4d0-5bb43d07616d
imnasnainaec
force-pushed
the
copilot/fix-deployment-helm-charts
branch
from
e5421de to
5d9bc11
Compare
jasonleenaylor
left a comment
jasonleenaylor
left a comment
There was a problem hiding this comment.
@jasonleenaylor reviewed 15 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on copilot[bot]).
The deploy action used
kubectl set imagewhich only swaps container images — helm chart configs (secrets, schedules, ingress settings, etc.) were never updated. QA configs had drifted since Oct 2024, production since Apr 2025.Devin review: https://app.devin.ai/review/sillsdev/TheCombine/pull/4216
The
deploy_qaworkflow was successfully run from this branch: https://github.com/sillsdev/TheCombine/actions/runs/23814304234Changes
.github/actions/combine-deploy-update/action.ymlkubectl set imagesteps with a call todeploy/scripts/setup_combine.py, which runshelm upgradewith full chart configurationimage_registry_alias,update_cert_proxytarget(qaorprod) + all application secrets required bysetup_combine.py.github/workflows/deploy_qa.yml/deploy_release.ymltarget: qa/target: prodand all required secrets to the refactored actiondeploy_release.yml: simplified registry frompublic.ecr.aws+image_registry_alias: /thecombinetopublic.ecr.aws/thecombineOriginal prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.
This change is
Summary by CodeRabbit