diff --git a/.github/scripts/tests/cleanup-pr-artifacts.test.py b/.github/scripts/tests/cleanup-pr-artifacts.test.py index b5196b8..3bd5a26 100644 --- a/.github/scripts/tests/cleanup-pr-artifacts.test.py +++ b/.github/scripts/tests/cleanup-pr-artifacts.test.py @@ -308,7 +308,7 @@ def test_package_inventory_later_page_not_authorized_discards_partial_inventory( def test_listing_failures_in_each_family_and_level(self): for failing in ["package_type=container", "package_type=nuget", "container/image/versions?", "nuget/Cratis.Test/versions?"]: - statuses = [404, 429, 500] if failing.startswith("package_type=") else [401, 403, 404, 429, 500] + statuses = [404, 429, 500] if failing.startswith("package_type=") else [401, 404, 429, 500] for status in statuses: with self.subTest(failing=failing, status=status): api = FakeAPI() @@ -346,7 +346,42 @@ def test_bad_pagination_and_repeated_ids(self): def test_page_two_failure_prevents_deletion(self): api = FakeAPI() api.versions[("nuget", "Cratis.Test")] = [version(i) for i in range(1, 102)] - api.hook = lambda m, e: Response(status=403) if "page=2" in e else None + api.hook = lambda m, e: Response(status=500) if "page=2" in e else None + self.assert_failed_without_deletes(self.run_cleanup(api)) + + def unauthorized_package_api(self, failing): + api = FakeAPI() + api.packages["container"] = [package("container", "image")] + api.versions[("container", "image")] = [version(7, tags=["pr42"])] + [ + version(i, tags=["other"]) for i in range(1000, 1100)] + api.hook = lambda m, e: Response(status=403, raw=TOKEN.encode()) if failing in e else None + return api + + def test_package_version_inventory_403_skips_only_that_package(self): + for failing in ["container/image/versions?", "container/image/versions?state=active&per_page=100&page=2"]: + for mode in ["reusable", "dry-run"]: + with self.subTest(failing=failing, mode=mode): + api = self.unauthorized_package_api(failing) + status, plan, output, api = self.run_cleanup( + api, manual=mode == "dry-run", + expected_summary="Skipped 1 package(s) whose version inventory was not authorized") + self.assertEqual(status, 0, output) + self.assertIn("::warning::Skipped container package image", output) + self.assertNotIn("::error::", output) + self.assertEqual([t["package_name"] for t in plan["targets"]], ["Cratis.Test"]) + self.assertTrue(all("container" not in e for e in api.deletes)) + self.assertEqual(len(api.deletes), 0 if mode == "dry-run" else 1) + + def test_manual_apply_with_skipped_package_fails_without_deletion(self): + approved = self.approved() + api = self.unauthorized_package_api("container/image/versions?") + result = self.run_cleanup(api, manual=True, **approved) + self.assert_failed_without_deletes(result) + self.assertIn("::error::Skipped 1 package(s)", result[2]) + + def test_package_403_outside_inventory_still_fails(self): + api = FakeAPI() + api.hook = lambda m, e: Response(status=403) if m == "GET" and e.endswith("nuget/Cratis.Test") else None self.assert_failed_without_deletes(self.run_cleanup(api)) def test_all_target_preflight_before_first_delete(self): diff --git a/.github/workflows/cleanup-pr-artifacts.yml b/.github/workflows/cleanup-pr-artifacts.yml index 68bc5ea..c45e961 100644 --- a/.github/workflows/cleanup-pr-artifacts.yml +++ b/.github/workflows/cleanup-pr-artifacts.yml @@ -81,6 +81,9 @@ jobs: class InventoryNotAuthorized(CleanupError): pass + class PackageNotAuthorized(CleanupError): + pass + def require(condition, message): if not condition: raise CleanupError(message) @@ -125,6 +128,12 @@ jobs: if (method == "GET" and endpoint.split("?", 1)[0] == "orgs/Cratis/packages" and status in (401, 403)): raise InventoryNotAuthorized("Package inventory not authorized") + # A 403 on one package or its versions means the token cannot see that + # package; inventory skips it. Outside inventory it still fails. + if (method == "GET" and status == 403 + and re.fullmatch(r"orgs/Cratis/packages/[^/?]+/[^/?]+(/versions)?", + endpoint.split("?", 1)[0])): + raise PackageNotAuthorized(f"API {method} failed (HTTP {status})") require(status in allowed, f"API {method} failed (HTTP {status})") # Never log API bodies, headers, URLs from errors, or exception text. data = parse_json(response.read()) if status == 200 else None @@ -227,7 +236,7 @@ jobs: "version_name": version_name, "tags": tags, "delete_endpoint": endpoint, "restore_endpoint": endpoint + "/restore"} - def inventory(api, repository, pr): + def inventory(api, repository, pr, skipped): targets = [] for kind in ("container", "nuget"): print(f"Inventorying {kind} packages") @@ -240,7 +249,14 @@ jobs: names.add(name) if association(package) != repository: continue - for version in api.listing(package_endpoint(kind, name) + "/versions?state=active"): + try: + versions = api.listing(package_endpoint(kind, name) + "/versions?state=active") + except PackageNotAuthorized: + skipped.append(f"{kind}/{name}") + print(f"::warning::Skipped {kind} package {name}: version inventory not " + "authorized (HTTP 403). None of its versions were deleted.") + continue + for version in versions: target = target_for(version, kind, name, pr) if target: targets.append(target) @@ -293,8 +309,9 @@ jobs: api = API(token) if manual and not dry_run: verify_closed(api, repository, pr) + skipped = [] try: - plan = inventory(api, repository, pr) + plan = inventory(api, repository, pr, skipped) except InventoryNotAuthorized: message = ("Could not run: package inventory not authorized. No versions deleted. " "Create a classic PAT (fine-grained PATs are not supported) with " @@ -310,6 +327,15 @@ jobs: raise CleanupError(message) print(f"::warning::{message}") return + if skipped: + message = (f"Skipped {len(skipped)} package(s) whose version inventory was not " + f"authorized (HTTP 403): {', '.join(skipped)}. Their versions were not " + "deleted; give PACKAGE_CLEANUP_TOKEN admin access to them to clean them up.") + if os.environ.get("GITHUB_STEP_SUMMARY"): + with open(os.environ["GITHUB_STEP_SUMMARY"], "a") as summary: + summary.write(f"### PR package cleanup\n\n{message}\n") + if expected is not None: + raise CleanupError(message) if expected is not None: # Comparing canonical JSON also distinguishes booleans from numeric IDs. require(json.dumps(expected, sort_keys=True) == json.dumps(plan, sort_keys=True), diff --git a/README.md b/README.md index d3ad791..85901d1 100644 --- a/README.md +++ b/README.md @@ -502,7 +502,10 @@ caller. Callers with an explicit secrets map must add that pass only `PAT_WORKFLOWS` are unaffected. Permission failures remain blocking except for the organization inventory -HTTP 401/403 warning described above. HTTP 401 also covers an expired or revoked +HTTP 401/403 warning described above and an HTTP 403 on one package's version +inventory: cleanup warns, skips that package without deleting any of its +versions, and cleans up the others. Manual apply fails instead when a package was +skipped. HTTP 401 also covers an expired or revoked token: it now produces a warning in reusable cleanup calls and dry runs, but manual apply and other workflows using that secret still fail.