feat(builds): Pull + Commit + Push Builds #6498
Open
Aradhya-Tripathi wants to merge 11 commits into
Open
Conversation
Aradhya-Tripathi
requested review from
balamurali27,
siduck,
ssiyad and
tanmoysrt
as code owners
Contributor
|
| Filename | Overview |
|---|---|
| press/press/doctype/bench_update/bench_update.py | Adds trigger_patch_deploy parameter to deploy(); accessing deploy["name"] after trigger_patch_deploy() returns an error dict (no name key) when builds are suspended causes a KeyError. Also, the parameter is a dead letter in the new-deploy-flow path. |
| press/press/doctype/release_pipeline/release_pipeline.py | Adds initiate_patch_deploy task and trigger_patch_deploy threading through prepare_deployment/create_deploy_candidate; initiate_patch_deploy accesses ["name"] on a dict that has no name key when builds are suspended. |
| dashboard/src/components/group/UpdateReleaseGroupDialog.vue | Adds Deploy as Patch button and patchDeploy resource; deployAsPatch() method lacks the restrictMessage guard that updateBench() enforces, allowing bypass of the I understand confirmation UI. |
| press/press/doctype/deploy_candidate_build/deploy_candidate_build.py | Adds patch build lifecycle methods; logic is well-structured with proper exception handling. |
| press/press/doctype/release_group/release_group.py | Adds _get_previous_candidate, _has_active_benches, and can_run_patch_build helpers; checks are comprehensive across apps, sources, deps, packages, and envvars. |
| press/press/doctype/deploy_candidate/deploy_candidate.py | Adds trigger_patch_deploy() whitelisted method; correctly checks is_suspended() but returns an error dict instead of throwing, which callers must handle explicitly. |
| press/api/bench.py | Threads trigger_patch_deploy flag through deploy_and_update into both old and new deploy flows; the old-flow path is where the suspension-check KeyError would surface. |
Sequence Diagram
sequenceDiagram
participant UI as UpdateReleaseGroupDialog
participant API as press.api.bench
participant BU as BenchUpdate.deploy()
participant DC as DeployCandidate
participant DCB as DeployCandidateBuild
participant Agent as Agent (builder)
UI->>API: "deploy_and_update(trigger_patch_deploy=True)"
API->>BU: "bench_update.deploy(trigger_patch_deploy=True)"
BU->>DC: rg.create_deploy_candidate()
DC-->>BU: candidate
BU->>DC: candidate.trigger_patch_deploy()
Note over DC: Returns error dict if suspended - deploy[name] raises KeyError
DC-->>BU: "error=False, name=build_name"
BU-->>API: build_name
DCB->>DCB: after_insert - run_patch_build()
DCB->>DCB: can_run_patch_build() check
DCB->>DCB: _get_previous_candidate()
DCB->>Agent: run_patch_build(base_image, app_instructions)
Agent-->>DCB: job callback process_run_patch_build
DCB->>DCB: _sync_patch_build_step_statuses()
DCB->>DCB: patch_build_output_parser.parse_and_update()
alt SUCCESS
DCB->>DCB: update_deploy_candidate_with_build()
DCB->>DCB: _create_platform_patch_build_if_required_and_deploy()
else FAILURE
DCB->>DCB: handle_build_failure()
end
Prompt To Fix All With AI
Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
press/press/doctype/bench_update/bench_update.py:116-122
`KeyError` when builds are suspended and a patch deploy is requested. `trigger_patch_deploy()` returns `{"error": True, "message": "..."}` — which has no `"name"` key — when `is_suspended()` is `True`, so `deploy["name"]` on the next line raises an unhandled `KeyError` that bubbles up to the API caller as a 500 error.
```suggestion
deploy = (
candidate.schedule_build_and_deploy(ignore_permissions=ignore_permissions)
if not trigger_patch_deploy
else candidate.trigger_patch_deploy(ignore_permissions=ignore_permissions)
)
if deploy.get("error"):
frappe.throw(deploy.get("message", "Patch build could not be initiated."))
return deploy["name"]
```
### Issue 2 of 4
press/press/doctype/release_pipeline/release_pipeline.py:293-298
`trigger_patch_deploy()` returns `{"error": True, "message": "..."}` — with no `"name"` key — when builds are suspended at task execution time. If builds are suspended in the narrow window between task enqueuing and execution, this line raises a `KeyError`, causing an opaque task failure instead of a clear suspension message propagating through the pipeline.
```suggestion
@task(queue=_get_task_execution_queue())
def initiate_patch_deploy(self, deploy_candidate: str) -> str:
"""Start the deploy candidate build process with patch deploy flag, skipping pre-build validations."""
candidate: DeployCandidate = frappe.get_doc("Deploy Candidate", deploy_candidate)
deploy_candidate_build = candidate.trigger_patch_deploy(ignore_permissions=True)
if deploy_candidate_build.get("error"):
raise ReleasePipelineFailure(
deploy_candidate_build.get("message", "Patch build could not be initiated.")
)
return deploy_candidate_build["name"]
```
### Issue 3 of 4
dashboard/src/components/group/UpdateReleaseGroupDialog.vue:739-741
`deployAsPatch()` skips the same pre-submit checks that `updateBench()` enforces. When `canShowDeploy` is true while `step === 'restrict-build'`, both buttons are visible but only the regular deploy button guards against `restrictMessage && !ignoreWillFailCheck`. A user can click "Deploy as Patch" and bypass the UI confirmation; the backend re-runs the will-fail check but the asymmetry could allow patch deploys to slip through if the check passes on retry.
```suggestion
deployAsPatch() {
if (this.restrictMessage && !this.ignoreWillFailCheck) {
this.errorMessage =
'Please check the <b>I understand</b> box to proceed'
return
}
this.$resources.patchDeploy.submit()
},
```
### Issue 4 of 4
press/press/doctype/bench_update/bench_update.py:116-122
**Dead-letter `trigger_patch_deploy` in new-deploy-flow path**
`create_deploy_candidate()` in `release_pipeline.py` calls `bench_update.deploy(create_build=False, trigger_patch_deploy=trigger_patch_deploy)`. Because `create_build=False` causes an early return before the `trigger_patch_deploy` branch is reached, the parameter has no effect here — the pipeline triggers the patch deploy explicitly via `initiate_patch_deploy()` instead. The silently ignored parameter could mislead future readers into thinking the pipeline already handles dispatch here.
Reviews (4): Last reviewed commit: "Merge branch 'develop' into quick-builds" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (37.15%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #6498 +/- ##
===========================================
- Coverage 50.05% 50.03% -0.03%
===========================================
Files 954 954
Lines 78960 79147 +187
Branches 366 369 +3
===========================================
+ Hits 39527 39603 +76
- Misses 39408 39518 +110
- Partials 25 26 +1
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:
|
Contributor
Author
|
@greptileai rereview |
Contributor
Author
|
@greptileai rereview |
Contributor
Author
The second issue is not correct, since the secondary platform build will not create a new deploy candidate doctype instead will associate the deploy_Candidate_build doctype to the older deploy_candidate doctype |
Contributor
Author
|
@greptileai rereview |
Comment on lines
+116
to
122
| deploy = ( | ||
| candidate.schedule_build_and_deploy(ignore_permissions=ignore_permissions) | ||
| if not trigger_patch_deploy | ||
| else candidate.trigger_patch_deploy(ignore_permissions=ignore_permissions) | ||
| ) | ||
|
|
||
| return deploy["name"] |
Contributor
There was a problem hiding this comment.
KeyError when builds are suspended and a patch deploy is requested. trigger_patch_deploy() returns {"error": True, "message": "..."} — which has no "name" key — when is_suspended() is True, so deploy["name"] on the next line raises an unhandled KeyError that bubbles up to the API caller as a 500 error.
Suggested change
| deploy = ( | |
| candidate.schedule_build_and_deploy(ignore_permissions=ignore_permissions) | |
| if not trigger_patch_deploy | |
| else candidate.trigger_patch_deploy(ignore_permissions=ignore_permissions) | |
| ) | |
| return deploy["name"] | |
| deploy = ( | |
| candidate.schedule_build_and_deploy(ignore_permissions=ignore_permissions) | |
| if not trigger_patch_deploy | |
| else candidate.trigger_patch_deploy(ignore_permissions=ignore_permissions) | |
| ) | |
| if deploy.get("error"): | |
| frappe.throw(deploy.get("message", "Patch build could not be initiated.")) | |
| return deploy["name"] |
Prompt To Fix With AI
This is a comment left during a code review.
Path: press/press/doctype/bench_update/bench_update.py
Line: 116-122
Comment:
`KeyError` when builds are suspended and a patch deploy is requested. `trigger_patch_deploy()` returns `{"error": True, "message": "..."}` — which has no `"name"` key — when `is_suspended()` is `True`, so `deploy["name"]` on the next line raises an unhandled `KeyError` that bubbles up to the API caller as a 500 error.
```suggestion
deploy = (
candidate.schedule_build_and_deploy(ignore_permissions=ignore_permissions)
if not trigger_patch_deploy
else candidate.trigger_patch_deploy(ignore_permissions=ignore_permissions)
)
if deploy.get("error"):
frappe.throw(deploy.get("message", "Patch build could not be initiated."))
return deploy["name"]
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+293
to
+298
| @task(queue=_get_task_execution_queue()) | ||
| def initiate_patch_deploy(self, deploy_candidate: str) -> str: | ||
| """Start the deploy candidate build process with patch deploy flag, skipping pre-build validations.""" | ||
| candidate: DeployCandidate = frappe.get_doc("Deploy Candidate", deploy_candidate) | ||
| deploy_candidate_build = candidate.trigger_patch_deploy(ignore_permissions=True) | ||
| return deploy_candidate_build["name"] |
Contributor
There was a problem hiding this comment.
trigger_patch_deploy() returns {"error": True, "message": "..."} — with no "name" key — when builds are suspended at task execution time. If builds are suspended in the narrow window between task enqueuing and execution, this line raises a KeyError, causing an opaque task failure instead of a clear suspension message propagating through the pipeline.
Suggested change
| @task(queue=_get_task_execution_queue()) | |
| def initiate_patch_deploy(self, deploy_candidate: str) -> str: | |
| """Start the deploy candidate build process with patch deploy flag, skipping pre-build validations.""" | |
| candidate: DeployCandidate = frappe.get_doc("Deploy Candidate", deploy_candidate) | |
| deploy_candidate_build = candidate.trigger_patch_deploy(ignore_permissions=True) | |
| return deploy_candidate_build["name"] | |
| @task(queue=_get_task_execution_queue()) | |
| def initiate_patch_deploy(self, deploy_candidate: str) -> str: | |
| """Start the deploy candidate build process with patch deploy flag, skipping pre-build validations.""" | |
| candidate: DeployCandidate = frappe.get_doc("Deploy Candidate", deploy_candidate) | |
| deploy_candidate_build = candidate.trigger_patch_deploy(ignore_permissions=True) | |
| if deploy_candidate_build.get("error"): | |
| raise ReleasePipelineFailure( | |
| deploy_candidate_build.get("message", "Patch build could not be initiated.") | |
| ) | |
| return deploy_candidate_build["name"] |
Prompt To Fix With AI
This is a comment left during a code review.
Path: press/press/doctype/release_pipeline/release_pipeline.py
Line: 293-298
Comment:
`trigger_patch_deploy()` returns `{"error": True, "message": "..."}` — with no `"name"` key — when builds are suspended at task execution time. If builds are suspended in the narrow window between task enqueuing and execution, this line raises a `KeyError`, causing an opaque task failure instead of a clear suspension message propagating through the pipeline.
```suggestion
@task(queue=_get_task_execution_queue())
def initiate_patch_deploy(self, deploy_candidate: str) -> str:
"""Start the deploy candidate build process with patch deploy flag, skipping pre-build validations."""
candidate: DeployCandidate = frappe.get_doc("Deploy Candidate", deploy_candidate)
deploy_candidate_build = candidate.trigger_patch_deploy(ignore_permissions=True)
if deploy_candidate_build.get("error"):
raise ReleasePipelineFailure(
deploy_candidate_build.get("message", "Patch build could not be initiated.")
)
return deploy_candidate_build["name"]
```
How can I resolve this? If you propose a fix, please make it concise.
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
1 participant
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.
No description provided.