Skip to content

uplift_worker: disable auto try push for secure uplifts (bug 2053811)#1329

Open
Magnolia-Liu wants to merge 21 commits into
mainfrom
magnolia/bug-2053811-disable-sec-uplift-auto-try-push
Open

uplift_worker: disable auto try push for secure uplifts (bug 2053811)#1329
Magnolia-Liu wants to merge 21 commits into
mainfrom
magnolia/bug-2053811-disable-sec-uplift-auto-try-push

Conversation

@Magnolia-Liu

@Magnolia-Liu Magnolia-Liu commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Automatic try-pushes for for security uplift requests currently give an error sent in a email notification.

Avoid automatically pushing to try for security uplifts by adding a new is_secure boolean field in UpliftSubmission model, setting the status in the uplift post request, detecting this status in the uplift worker, and skipping the try push based on this detection.


Lando: link
Bugzilla: bug 2053811

⚠️ This pull request has 7 warnings.
🚫 This pull request has 1 blocker.

@Magnolia-Liu
Magnolia-Liu requested a review from zzzeid July 10, 2026 15:44
@github-actions

Copy link
Copy Markdown

View this pull request in Lando to land it once approved.

@Magnolia-Liu Magnolia-Liu changed the title uplift_worker: disable auto try push for secure uplifts uplift_worker: disable auto try push for secure uplifts (bug 2053811) Jul 10, 2026
@Magnolia-Liu
Magnolia-Liu marked this pull request as ready for review July 13, 2026 13:25
@Magnolia-Liu
Magnolia-Liu requested a review from a team as a code owner July 13, 2026 13:25
@Magnolia-Liu
Magnolia-Liu marked this pull request as draft July 13, 2026 15:15
@Magnolia-Liu
Magnolia-Liu marked this pull request as ready for review July 14, 2026 14:19
Comment thread src/lando/api/tests/test_uplift.py Outdated
Comment thread src/lando/api/tests/test_uplift.py
Comment thread src/lando/api/tests/test_uplift.py Outdated

@cgsheeh cgsheeh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should use BugReferencesCheck in the worker here instead of using revision_is_secure to check the Phabricator secure-project tag. This gate is really about whether a commit references a private bug, so reusing the check skips try only when the push would actually break/leak, preserves CI coverage for secure-tagged revisions that reference public bugs (common when phab-bot is stalled), and survives the GitHub migration since it only needs commit messages + BMO.

@shtrom

shtrom commented Jul 15, 2026

Copy link
Copy Markdown
Member

I think we should use BugReferencesCheck in the worker here instead of using revision_is_secure to check the Phabricator secure-project tag. This gate is really about whether a commit references a private bug, so reusing the check skips try only when the push would actually break/leak, preserves CI coverage for secure-tagged revisions that reference public bugs (common when phab-bot is stalled), and survives the GitHub migration since it only needs commit messages + BMO.

Perhaps we should apply all of the landing checks enabled for the target repo?

@zzzeid

zzzeid commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

I think we should use BugReferencesCheck in the worker here instead of using revision_is_secure to check the Phabricator secure-project tag. This gate is really about whether a commit references a private bug, so reusing the check skips try only when the push would actually break/leak, preserves CI coverage for secure-tagged revisions that reference public bugs (common when phab-bot is stalled), and survives the GitHub migration since it only needs commit messages + BMO.

Perhaps we should apply all of the landing checks enabled for the target repo?

Yeah good point, this would also prevent hard-coding the check name, which would be good in case we ever add any other additional checks that affect security patches.

It would be good to update the docstrings and helptext of the hooks to make it more obvious that those are no longer only running at landing time.

Edit: removed comment (see more detail in bug 2056365)

@Magnolia-Liu
Magnolia-Liu requested review from cgsheeh and zzzeid July 20, 2026 21:16
Comment thread src/lando/api/legacy/workers/uplift_worker.py
Comment thread src/lando/api/legacy/workers/uplift_worker.py Outdated
Comment thread src/lando/api/legacy/workers/uplift_worker.py Outdated
Comment thread src/lando/main/models/uplift.py Outdated
Comment thread src/lando/ui/legacy/revisions.py
Comment thread src/lando/api/legacy/workers/uplift_worker.py Outdated
Comment thread src/lando/main/migrations/0063_upliftsubmission_is_secure.py Outdated
@Magnolia-Liu
Magnolia-Liu requested a review from zzzeid July 22, 2026 15:55
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.

4 participants