Skip to content

Commit 5a99164

Browse files
authored
Remove stale PR preview cleanup (#4721)
This reverts the stale PR preview cleanup. This feature removed previews for open PRs that have been inactive for over two weeks. We decided to remove it for the following reasons: 1. We originally introduced this to improve the reliability and speed of preview deployments. However, we're not sure it makes a noticeable difference; there are usually only one or two stale previews at any one point in time (vs ~20-30 previews in total). 2. Even with the small number of stale previews, the expiration has inconvenienced writers who needed to share previews with stakeholders. 3. The final straw is that the tests are broken in a really nasty way: They fail on the GitHub actions MacOS runner but pass on our local MacOS machines. When I deleted the failing test to debug things, the Windows runner started showing even more crazy errors. I have no idea what's causing it but it's related to the `datetime` library and I think it's going to take a lot of time to fix. I also think there's a real risk the code is actually broken and it's not just a test problem. With points 1 & 2, we decided to just remove the feature.
1 parent d102d7a commit 5a99164

File tree

2 files changed

+8
-131
lines changed

2 files changed

+8
-131
lines changed

scripts/pr-previews/cleanup.py

Lines changed: 7 additions & 128 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,7 @@
1515
import json
1616
import logging
1717
import shutil
18-
import time
1918
from pathlib import Path
20-
from datetime import datetime
21-
from typing import TypedDict
2219

2320
logger = logging.getLogger(__name__)
2421

@@ -32,9 +29,6 @@
3229

3330
INITIAL_COMMIT = "499a5040585d02593cdd8237e19c9ee4a84ae126"
3431

35-
SEVEN_DAYS_IN_SECONDS = 60 * 60 * 24 * 7
36-
PR_EXPIRATION_TIME_SECONDS = SEVEN_DAYS_IN_SECONDS
37-
3832

3933
def main() -> None:
4034
setup_git_account()
@@ -63,137 +57,22 @@ def main() -> None:
6357
logger.info("Cleaned up closed PR previews")
6458

6559

66-
class PrFolders(TypedDict):
67-
all_open: set[str]
68-
open_stale: set[str]
69-
70-
71-
def determine_stale(
72-
api_response: str, current_time: int, expiration_period: int
73-
) -> PrFolders:
74-
# We parse the JSON into `{ number: int, updatedAt: int }[]`
75-
def parse_updated_at(data):
76-
"""Converts "updatedAt" fields from iso strings into unix timestamps"""
77-
if "updatedAt" in data:
78-
iso_format = data["updatedAt"]
79-
timestamp = datetime.fromisoformat(iso_format).timestamp()
80-
data["updatedAt"] = timestamp
81-
return data
82-
83-
parsed = json.loads(api_response, object_hook=parse_updated_at)
84-
all_pr_numbers = (pr["number"] for pr in parsed)
85-
86-
# ========================================================
87-
# Extract stale PRs
88-
# ========================================================
89-
90-
def is_stale(updated_at: int) -> bool:
91-
return (current_time - updated_at) > expiration_period
92-
93-
stale_pr_numbers = (pr["number"] for pr in parsed if is_stale(pr["updatedAt"]))
94-
95-
# === Return folder names ===
96-
def number_to_folder(num: int) -> str:
97-
return f"pr-{num}"
98-
99-
return {
100-
"all_open": set(map(number_to_folder, all_pr_numbers)),
101-
"open_stale": set(map(number_to_folder, stale_pr_numbers)),
102-
}
103-
104-
105-
def get_pr_folders() -> PrFolders:
106-
api_response = run_subprocess(
107-
[
108-
"gh",
109-
"pr",
110-
"list",
111-
"--state",
112-
"open",
113-
"--json",
114-
"number,updatedAt",
115-
"--limit",
116-
"1000",
117-
]
60+
def get_active_pr_folders() -> 'set[str]':
61+
raw = run_subprocess(
62+
["gh", "pr", "list", "--state", "open", "--json", "number", "--limit", "1000"]
11863
).stdout
119-
return determine_stale(api_response, int(time.time()), PR_EXPIRATION_TIME_SECONDS)
120-
64+
# `raw` is JSON string of form: { number: int }[]
65+
return {f"pr-{obj['number']}" for obj in json.loads(raw)}
12166

12267
def delete_closed_pr_folders() -> None:
123-
pr_folders = get_pr_folders()
124-
68+
active_pr_folders = get_active_pr_folders()
12569
for folder in Path(".").glob("pr-*"):
126-
is_closed = folder.name not in pr_folders["all_open"]
70+
is_closed = folder.name not in active_pr_folders
12771
if is_closed:
12872
logger.info(f"Deleting {folder} as PR is closed")
12973
shutil.rmtree(folder)
130-
continue
131-
132-
is_stale = folder.name in pr_folders["open_stale"]
133-
if is_stale:
134-
logger.info(f"Deleting {folder} as it is stale")
135-
shutil.rmtree(folder)
136-
continue
13774

13875

13976
if __name__ == "__main__":
14077
configure_logging()
14178
main()
142-
143-
144-
# =====
145-
# TESTS
146-
# =====
147-
148-
149-
def test_determine_stale_empty_list():
150-
assert determine_stale("[]", 0, 1) == {"all_open": set(), "open_stale": set()}
151-
152-
153-
def test_determine_stale_active_pr():
154-
api_response = """[{
155-
"number": 1,
156-
"updatedAt": "1970-01-01T01:00:00"
157-
}]"""
158-
assert determine_stale(api_response, current_time=0, expiration_period=1) == {
159-
"all_open": {"pr-1"},
160-
"open_stale": set(),
161-
}
162-
163-
164-
def test_determine_stale_stale_pr():
165-
# unix_epoch = "1970-01-01T01:00:00"
166-
# (checked wih `datetime.fromtimestamp(0).isoformat()`)
167-
api_response = """[{
168-
"number": 1,
169-
"updatedAt": "1970-01-01T01:00:00"
170-
}]"""
171-
assert determine_stale(api_response, current_time=100, expiration_period=10) == {
172-
"all_open": {"pr-1"},
173-
"open_stale": {"pr-1"},
174-
}
175-
176-
177-
def test_determine_stale_many_results():
178-
api_response = """[
179-
{
180-
"number": 1,
181-
"updatedAt": "1970-01-01T01:00:00"
182-
},
183-
{
184-
"number": 2,
185-
"updatedAt": "1970-01-02T01:00:00"
186-
},
187-
{
188-
"number": 3,
189-
"updatedAt": "1970-01-03T01:00:00"
190-
}
191-
]"""
192-
current_time = int(datetime(1970, 1, 4, 1, 0).timestamp()) # 4th Jan, 1970
193-
two_days_in_seconds = 60 * 60 * 24 * 2
194-
assert determine_stale(
195-
api_response, current_time, expiration_period=two_days_in_seconds
196-
) == {
197-
"all_open": {"pr-1", "pr-2", "pr-3"},
198-
"open_stale": {"pr-1"},
199-
}

tox.ini

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,4 @@ deps =
3434
-e scripts/image-tester
3535
-e scripts/notebook-normalizer
3636
pytest
37-
commands =
38-
pytest
39-
pytest scripts/pr-previews/cleanup.py
37+
commands = pytest

0 commit comments

Comments
 (0)