[WPT Export] If a CL no longer has exportable changes the upstream PR is forgotten |
||||
Issue descriptionCurrently 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
,
Aug 1 2017
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
,
Aug 3 2017
Issue 747038 has been merged into this issue.
,
Aug 4 2017
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.
,
Aug 4 2017
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.
,
Oct 23 2017
Issue 774782 has been merged into this issue.
,
Oct 24
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
,
Oct 25
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?
,
Oct 25
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?
,
Oct 26
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.
,
Nov 27
,
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 |
||||
Comment 1 by foolip@chromium.org
, Aug 1 2017