New issue
Advanced search Search tips

Issue 750942 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 27
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 707006



Sign in to add a comment

[WPT Export] If a CL no longer has exportable changes the upstream PR is forgotten

Project Member Reported by jeffcarp@chromium.org, Aug 1 2017

Issue description

Currently if a CL has exportable changes then uploads new patches removing all exportable changes, the corresponding upstream PR will not be touched and will be forgotten by the exporter.

This is hard to detect since when the CL lands on master, its commit won't be considered exportable, so the exporter won't ever deal with the PR. One solution could be to have the Exporter do a separate loop over all open PRs to clean up any stale ones.

However this may be enough of an edge case that manual intervention by the Ecosystem Infra oncall is more preferable than adding new complexity to the exporter. Currently that's my opinion, but I'd like to hear yours.

Via https://chromium-review.googlesource.com/c/580990/#message-72cd84ee6abcefc4f232e0e6c90ab7ce7c827791
 
What would be the manual steps for finding and closing these PRs?
The manual way that I found a case before was to go through the list of PRs with the label "chromium-export", from old to new, and for each one click on the "Reviewed on" link to check if the corresponding CL is closed or not.

PR list: https://github.com/w3c/web-platform-tests/pulls?q=is%3Apr+is%3Aopen+label%3Achromium-export
 Issue 747038  has been merged into this issue.
If we want to automatize the process, we can iterate over the list of open PRs with the export label, which the bot already fetches currently, and check the corresponding CL is still exportable. Should be tractable, but again it really depends on how often such scenarios happen.
One problem we should make sure to avoid is thrashing the PR if a CL goes between states frequently. If a CL, for example, goes:

exportable changes -> no exportable changes -> exportable changes -> no exportable changes

it will result in a lot of noise in the repo.
 Issue 774782  has been merged into this issue.
Project Member

Comment 7 by sheriffbot@chromium.org, Oct 24

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Owner: kyleju@chromium.org
Status: Assigned (was: Untriaged)
Kyle, do you think you can modify your PR cleanup tool for  issue 852014  to support this as well? Would it be an easy extension?
Seems to fit it with the type of stuff our cleanup tool does. My questions are
 
1) when a CL is considered exportable?
2) when a CL goes between states?
3) comment2 mentioned "check if the corresponding CL is closed or not". Do we treat a closed case the same as an abandoned case?

1) A CL is exportable if it contains exportable changes (basically, changes in external/wpt excluding baselines).
2) Sorry I'm not sure about your question. If you're asking exportable/un-exportable, then a CL can change between the two if new patchsets add/remove exportable changes. Besides, CL has a lifecycle of WIP -> in review -> closed (landed/abandoned), which are also "states".
3) So yeah, when a CL is closed (regardless of whether it is merged or abandoned), its exportable/un-exportable state finalizes, at which point we should check if we need to abandon the export PR.

Basically, in your cleanup script, we are already scanning the open export PRs to see if their original CLs are abandoned; we can also check if the original CLs are merged but without exportable changes, and close those PRs as well.
Status: Fixed (was: Assigned)
Project Member

Comment 12 by bugdroid1@chromium.org, Nov 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/46c9d5ef85ac910175eaeb1c4c44f708cde94f5f

commit 46c9d5ef85ac910175eaeb1c4c44f708cde94f5f
Author: kyle Ju <kyleju@chromium.org>
Date: Tue Nov 27 17:26:57 2018

Add an edge case to PR clenaup tool.

Address the edge case where when a CL no longer has exportable changes, the PR should
be closed and the branch should be deleted.


Bug:  852014 ,  750942 
Change-Id: I86adf85a3dcec7a9fac088fa42e2e357bbf1285a
Reviewed-on: https://chromium-review.googlesource.com/c/1349471
Commit-Queue: Robert Ma <robertma@chromium.org>
Reviewed-by: Robert Ma <robertma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611164}
[modify] https://crrev.com/46c9d5ef85ac910175eaeb1c4c44f708cde94f5f/third_party/blink/tools/blinkpy/w3c/pr_cleanup_tool.py
[modify] https://crrev.com/46c9d5ef85ac910175eaeb1c4c44f708cde94f5f/third_party/blink/tools/blinkpy/w3c/pr_cleanup_tool_unittest.py

Sign in to add a comment