Skip to content

fix(deploy): elevate to Administrator on force fail - #7456

Open
siduck wants to merge 3 commits into
developfrom
fix-ui
Open

fix(deploy): elevate to Administrator on force fail#7456
siduck wants to merge 3 commits into
developfrom
fix-ui

Conversation

@siduck

@siduck siduck commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

The earlier fix broke because it ran as the regular user, who isn't allowed to touch Agent Job records, so cancelling silently failed every time. Now it runs as admin for just that step

@greptile-apps

greptile-apps Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

Safe to merge; the previous test-coverage finding is fully fixed.

Reviews (3) · Last reviewed commit: "Merge branch 'develop' into fix-ui"

Comment on lines +235 to +240
frappe.set_user("Administrator")
try:
for pipeline_build in self.pipeline_builds:
self._cancel_build_if_running(pipeline_build.build)
finally:
frappe.set_user(current_user)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Regression Coverage Missing
This bug fix adds Administrator impersonation and restoration without a regression test. The repository requires every bug fix to have coverage, so add a test proving force-fail cancels builds as Administrator and restores the original user after both success and exceptions before merging.

Context Used: Guidelines for reviewing Frappe Framework applications. (source)

Prompt To Fix With AI
This is a comment left during a code review.
Path: press/press/doctype/release_pipeline/release_pipeline.py
Line: 235-240

Comment:
**Regression Coverage Missing**
This bug fix adds Administrator impersonation and restoration without a regression test. The repository requires every bug fix to have coverage, so add a test proving force-fail cancels builds as Administrator and restores the original user after both success and exceptions before merging.

**Context Used:** Guidelines for reviewing Frappe Framework applications. ([source](https://github.com/frappe/skills/blob/main/skills/quality-code-review/SKILL.md))

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@codecov-commenter

codecov-commenter commented Sep 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.82609% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 62.07%. Comparing base (07485b0) to head (d220ef1).
⚠️ Report is 24 commits behind head on develop.

Files with missing lines Patch % Lines
...press/doctype/release_pipeline/release_pipeline.py 92.85% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #7456      +/-   ##
===========================================
+ Coverage    61.94%   62.07%   +0.13%     
===========================================
  Files         1067     1067              
  Lines       102449   102679     +230     
  Branches      1777     1777              
===========================================
+ Hits         63460    63738     +278     
+ Misses       38944    38896      -48     
  Partials        45       45              
Flag Coverage Δ
dashboard 85.94% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mergify

mergify Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants