Skip to content

template-only-bin: Include additional resources in cleanup-test-resources#991

Closed
doshitan wants to merge 11 commits intomainfrom
doshitan/cleanup-script-tweaks
Closed

template-only-bin: Include additional resources in cleanup-test-resources#991
doshitan wants to merge 11 commits intomainfrom
doshitan/cleanup-script-tweaks

Conversation

@doshitan
Copy link
Copy Markdown
Contributor

Ticket

Related to #706

Changes

There were a number of resource types not covered by the script that were still left around after initial cleanup, so extend removal coverage to those. Not complete or perfect (e.g., there are a other networking resources that could prevent an VPC from being able to be removed), but covers what we've seen so far.

Context for reviewers

Will probably not squash when merging, but also didn't want to make 12 PRs. So best reviewed commit by commit for logical chunks.

Testing

--dry-run resource list before: clean-pre-results.txt

--dry-run resource list after: clean-post-final-tweaks.txt

Infra tests are now passing: https://github.com/navapbc/template-infra/actions/runs/21011517717/job/60517259169

@doshitan doshitan requested a review from sean-navapbc January 16, 2026 15:14
@doshitan doshitan requested a review from a team as a code owner January 16, 2026 15:14
@doshitan doshitan force-pushed the doshitan/cleanup-script-tweaks branch from eb82720 to f113af9 Compare January 16, 2026 15:35
Copy link
Copy Markdown
Contributor

@sean-navapbc sean-navapbc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvements to the cleanup script! The refactoring to use the already-fetched resources list instead of re-querying each
resource type is a good simplification. The new helper scripts (delete-iam-role, empty-s3-bucket, list-iam-roles-with-tag)
are well-structured.

Bug Found

Line 358 in cleanup-test-resources: There's a typo in the AWS CLI flag:

role_project_tag=$(aws iam list-role-tags \
    --role_name "${role_name}" \

Should be --role-name (hyphen) not --role_name (underscore). This will cause the IAM role tag lookup to fail.

Minor Suggestions

  1. SNS grep pattern (line 336): The pattern 'arn:aws:sns:.*' could match unintended resources. Consider being more specific
    with 'arn:aws:sns:.*:.*:' to ensure it only matches topic ARNs.

  2. VPC dependency order: The script deletes subnets and internet gateways before VPCs, which is correct. However, there may
    be other dependencies (NAT gateways, route tables with explicit associations) that could still block VPC deletion. The current
    || echo "Failed..." handling is fine for now, but worth noting for future iterations.

Verdict

Please fix the --role_name--role-name bug, then this is good to merge

@doshitan doshitan force-pushed the doshitan/cleanup-script-tweaks branch from f113af9 to d334b90 Compare January 16, 2026 18:40
@doshitan
Copy link
Copy Markdown
Contributor Author

Please fix the --role_name--role-name bug, then this is good to merge

Good catch! Done.

1. **SNS grep pattern** (line 336): The pattern `'arn:aws:sns:.*'` could match unintended resources. Consider being more specific
   with `'arn:aws:sns:.*:.*:'` to ensure it only matches topic ARNs.

Topics are the only SNS ARNs AFAIK, which I suppose could change in the future, but if it does this would need updated anyway. So I think it's fine as is.

@doshitan doshitan requested a review from sean-navapbc January 16, 2026 18:40
Copy link
Copy Markdown
Contributor

@sean-navapbc sean-navapbc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@doshitan
Copy link
Copy Markdown
Contributor Author

Manually merged as individual commits: 8e32d98...eb06b65

@doshitan doshitan closed this Feb 24, 2026
@doshitan doshitan deleted the doshitan/cleanup-script-tweaks branch February 24, 2026 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants