Skip to content

refactor: simplify cleanup workflow, deduplicate shell scripts, and add unit test suite - #1

Open
getdownandcode wants to merge 2 commits into
mainfrom
refactor/simplify-codebase
Open

refactor: simplify cleanup workflow, deduplicate shell scripts, and add unit test suite#1
getdownandcode wants to merge 2 commits into
mainfrom
refactor/simplify-codebase

Conversation

@getdownandcode

@getdownandcode getdownandcode commented Oct 9, 2026

Copy link
Copy Markdown
Owner

Overview

This PR simplifies and streamlines the Infra-Guard codebase by eliminating repetitive boilerplate across the core Python script and shell utilities, without changing any existing features, CLI flags, Prometheus metric names, or AWS behaviors.

Key Improvements

1. Unified Cleanup Engine (scripts/cleanup.py)

  • Before: 5 separate cleanup loops (ebs, elastic_ips, instances, snapshots, security_groups) duplicated tag checks, skip evaluations, apply_action calls, and counter tracking across 150+ lines.
  • After: Consolidated into a single run_cleanup engine. Each resource scanner is now a lightweight generator that yields (resource_id, description, action_callable, tags):
def run_cleanup(
    items: Iterable[Tuple[str, str, Callable[[], Any], Dict[str, str]]],
    dry_run: bool,
    tag_key: Optional[str],
    tag_value: Optional[str],
) -> Tuple[int, int]:
    acted, failed = 0, 0
    for res_id, desc, action, tags in items:
        if not matches_filter(tags, tag_key, tag_value):
            continue
        if tag_is_true(tags, SKIP_TAG):
            logging.info("Skipping %s because %s=true", res_id, SKIP_TAG)
            continue
        if apply_action(desc, dry_run, action):
            acted += 1
        else:
            failed += 1
    return acted, failed

2. Deduplicated Shell Scripts (scripts/bootstrap_s3.sh & scripts/s3-sync.sh)

  • Replaced 15 lines of nested if/else bucket resolution with concise Bash parameter expansion:
ACCOUNT_ID=$(aws sts get-caller-identity --query Account --output text 2>/dev/null || echo "")
S3_BUCKET=${S3_STATE_BUCKET:-"infra-guard-state${ACCOUNT_ID:+-$ACCOUNT_ID-$AWS_REGION}"}

3. Declarative Jenkinsfile (Jenkinsfile)

  • Streamlined the Apply stage shell command using native environment variable expansion, eliminating Groovy script blocks.
  • Added automated unit test discovery to the Lint stage.

4. Automated Unit Test Suite (tests/test_cleanup.py)

  • Added 16 automated tests covering in-memory logging, tag filters, age math, stopped instance parsing, resource scanners, and metric exports.

Verification & Test Output

All tests and syntax checks pass:

$ python3 -m unittest discover -s tests -v
test_apply_action_confirm_failure (test_cleanup.TestCleanupUtils) ... ok
test_apply_action_confirm_success (test_cleanup.TestCleanupUtils) ... ok
test_apply_action_dry_run (test_cleanup.TestCleanupUtils) ... ok
test_find_ebs_volumes (test_cleanup.TestCleanupUtils) ... ok
test_find_elastic_ips (test_cleanup.TestCleanupUtils) ... ok
test_find_orphaned_security_groups (test_cleanup.TestCleanupUtils) ... ok
test_find_stopped_instances (test_cleanup.TestCleanupUtils) ... ok
test_matches_filter (test_cleanup.TestCleanupUtils) ... ok
test_memory_log_handler (test_cleanup.TestCleanupUtils) ... ok
test_older_than (test_cleanup.TestCleanupUtils) ... ok
test_run_cleanup (test_cleanup.TestCleanupUtils) ... ok
test_stopped_at (test_cleanup.TestCleanupUtils) ... ok
test_tag_is_true (test_cleanup.TestCleanupUtils) ... ok
test_tags_to_dict (test_cleanup.TestCleanupUtils) ... ok
test_upload_log_to_s3 (test_cleanup.TestCleanupUtils) ... ok
test_write_prometheus_metrics (test_cleanup.TestCleanupUtils) ... ok

----------------------------------------------------------------------
Ran 16 tests in 0.005s

OK

$ python3 scripts/cleanup.py --help
usage: cleanup.py [-h] [--dry-run] [--confirm] [--region REGION]
                  [--log-bucket LOG_BUCKET] [--log-prefix LOG_PREFIX]
                  [--ebs-min-age-hours EBS_MIN_AGE_HOURS]
                  [--stopped-instance-min-age-days STOPPED_INSTANCE_MIN_AGE_DAYS]
                  [--snapshot-min-age-days SNAPSHOT_MIN_AGE_DAYS]
                  [--resource-tag-key RESOURCE_TAG_KEY]
                  [--resource-tag-value RESOURCE_TAG_VALUE]
                  [--metrics-file METRICS_FILE]

@getdownandcode
Refactor and simplify codebase: deduplicate logic, streamline scripts…
0b91e3e 
…, and add unit test suite

- scripts/cleanup.py: Consolidate 5 duplicated cleanup loops into a single run_cleanup processor and generator scanners, reducing ~140 lines of boilerplate while preserving all CLI flags, Prometheus metrics, and S3 logging
- scripts/bootstrap_s3.sh & s3-sync.sh: Eliminate duplicated bucket resolution and streamline region argument handling using concise parameter expansion
- Jenkinsfile: Streamline Apply stage shell parameter expansion and add unit test discovery to Lint stage
- tests/test_cleanup.py: Add 16 automated unit tests covering MemoryLogHandler, tag filters, age calculations, resource scanners, and metric exports
@getdownandcode
chore: ignore future improvements and internal planning documents
056d2c5
@getdownandcode

getdownandcode commented Oct 22, 2026

Copy link
Copy Markdown
Owner Author

🤖 Automated PR Review: #1 - refactor: simplify cleanup workflow, deduplicate shell scripts, and add unit test suite

1. Summary of Changes

  • Scope: Consolidates 5 duplicate resource cleanup loops into a single generator-based run_cleanup engine in scripts/cleanup.py, simplifies S3 shell scripts with Bash parameter expansions, and adds 16 unit tests.
  • Key Files Modified: scripts/cleanup.py, scripts/bootstrap_s3.sh, scripts/s3-sync.sh, Jenkinsfile, tests/test_cleanup.py

2. Analysis & Key Highlights

  • Strengths: Clean generator pattern (find_*), safe lambda closure bindings (vid=vid), and zero breaking changes to CLI flags or Prometheus metrics.
  • Observations: All 16 automated tests pass cleanly (0.006s).

3. Actionable Suggestions & Concerns

  • [Severity: Low] [scripts/cleanup.py:165]: Use (m.get("Ebs") or {}).get("SnapshotId") instead of m.get("Ebs", {}).get(...) in find_old_snapshots to guard against potential None values on non-EBS mappings.
  • [Severity: Low] [scripts/cleanup.py:340]: Add an explicit check if boto3 is None: sys.exit("boto3 required") in main() for clearer CLI feedback if AWS SDK is not installed.

4. Merge Status

Verdict: READY TO MERGE
Reasoning: Clean refactor, full backwards compatibility preserved, and all 16 unit tests pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

See More

Successfully merging this pull request may close these issues.

1 participant

@getdownandcode