pabloem commented on code in PR #39538:
URL: https://github.com/apache/beam/pull/39538#discussion_r3672635005
##########
.test-infra/tools/stale_cleaner.py:
##########
@@ -337,8 +337,11 @@ def _active_resources(self) -> dict:
subscription_name = subscription.name
# Apply prefix filtering if prefixes are defined
if not self.prefixes or
any(subscription_name.startswith(f"{self.project_path}/subscriptions/{prefix}")
for prefix in self.prefixes):
- # Check if the subscription has a topic associated with it
- if subscription.detached:
+ # Safe orphan detection:
Review Comment:
we need to be thoughtful about the behavior of this function.
The behavior before this change is:
- No prefixes are defined, so iterate through ALL subscriptions, and capture
only detached ones. Is that in line with your understanding?
If we apply this change, we:
- No longer catch all detached subscriptions because prefixes are defined.
Does that make sense?
- We are also adding a special case `if taxi...` which should be covered by
prefixes. This is turns the code into a totally strange thing.
So we have to change this function to something like this:
```
for s in list_subscriptions:
if subscription.detached = true:
d[s....]...
elif
any(subscription_name.startswith(f"{self.project_path}/subscriptions/{prefix}")
for prefix in self.prefixes):
d[s...]...
```
this covers our cases properly. Please go ahead and make that change.
##########
.test-infra/tools/test_stale_cleaner.py:
##########
@@ -433,26 +433,38 @@ def test_init(self):
def test_active_resources(self):
"""Test _active_resources method."""
+ # Keep the existing standard prefix coverage and add the taxi-only
cleanup prefix.
Review Comment:
can you confirm that you ran this test? also, mark one sub with
detached=True to make sure that case is covered.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
