Chore/cleanup

[pgoell]

No description provided.

[pgoell]

/review

[claude]

Pull Request Review: Chore/cleanup

Summary

This PR includes several cleanup and improvement changes:

• Adds GitHub Actions CI workflow for automated testing
• Removes pytest from pre-commit hooks (moved to CI)
• Adds pyyaml as a runtime dependency
• Adds E2E tests for git installation
• Removes PyPI badge from README
• Adds linter exceptions for subprocess security warnings in tests

---

Positive Changes

1. GitHub Actions Workflow

• Excellent: Multi-version testing across Python 3.10, 3.11, and 3.12
• Good: Properly uses astral-sh/setup-uv@v4 for uv installation
• Good: Separates test execution into unit, integration, and e2e stages

2. Pre-commit Hook Optimization

• Smart move: Removing pytest from pre-commit hooks improves developer experience
• Running tests on every commit is slow; CI is the right place for this

3. Critical Bug Fix (pyproject.toml + uv.lock)

• Essential: Adding pyyaml>=6.0 as a runtime dependency fixes a missing dependency issue
• The code uses yaml.safe_load() and yaml.dump() in modules/dbt/profile/models.py
• This was likely causing installation failures (as noted in the xfail markers)

4. E2E Test for Git Installation

• Great coverage: Tests both uv and pip installation from git tags
• Good practice: Uses @pytest.mark.xfail with clear explanation for known v1.3.0 issue

---

Issues & Concerns

1. Security Linter Exceptions (pyproject.toml:83) - MEDIUM SEVERITY

Adding S603 and S607 blanket exceptions for all tests is too broad. These are subprocess security checks. Instead of disabling globally, either use inline suppressions or scope to e2e tests only.

2. Subprocess Security - LOW SEVERITY

Add timeout parameter to all subprocess.run() calls to prevent hanging (e.g., 30-60 seconds).

3. Test Reliability - MEDIUM SEVERITY

The get_latest_tag() function at line 23 will crash on non-semver tags. Add validation and error handling for malformed tags.

4. GitHub Actions - MINOR

Consider adding dependency caching and coverage reporting for better performance and visibility.

---

Summary & Recommendations

Approve with minor changes requested:

1. MUST FIX: Narrow the scope of S603/S607 linter exceptions
2. SHOULD FIX: Add timeout parameters to subprocess calls
3. SHOULD FIX: Add better error handling in get_latest_tag()
4. NICE TO HAVE: Add dependency caching to GitHub Actions workflow

Overall Assessment: Solid cleanup PR that fixes a critical dependency issue (pyyaml) and improves CI/CD. Main concerns are overly broad security linter exceptions.

Test Coverage: Good - new E2E tests validate the exact issue being fixed

Mergeability: Recommend addressing the linter exception scope before merging