Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ WORKDIR /app
# Install system dependencies if any needed (e.g. for psycopg2)
# but psycopg2-binary is used, so slim should be enough.

COPY pyproject.toml .
COPY pyproject.toml README.md ./
RUN pip install --no-cache-dir .

COPY pipelineprobe/ ./pipelineprobe/
Expand Down
207 changes: 91 additions & 116 deletions docs/bugs.md
Original file line number Diff line number Diff line change
@@ -1,165 +1,140 @@
All 5 previous bugs are confirmed fixed. Here's the full state assessment:
Excellent progress — every single bug and issue from the last round is now confirmed fixed. Here is the honest current state:

***

## ✅ All Previous Bugs — Confirmed Fixed
## ✅ All Previous Issues — Confirmed Fixed

| Bug | Fix Status |
|-----|-----------|
| `asyncio.run()` crash → replaced with `psycopg2` sync | ✅ Fixed |
| `init` YAML key `url``base_url` | ✅ Fixed |
| `postgres_tables` hardcoded key → `warehouse_tables` + `warehouse_type` | ✅ Fixed |
| dbt manifest double-join path | ✅ Fixed — now `Path(self.config.manifest_path)` directly |
| `timedelta.days` truncation → `total_seconds() / 86400` | ✅ Fixed |
| `fail_on_critical` default mismatch → now `5` in both config and init YAML | ✅ Fixed |
| `report.html` template exists and renders | ✅ Confirmed |
| Item | Status |
|------|--------|
| `days_since` float in message → `:.1f` | ✅ Fixed |
| All-failed DAG silent missnow `critical` issue | ✅ Fixed |
| `[dev]` extras in `pyproject.toml` | ✅ Fixed |
| Hardcoded `"admin"` password → `None` + explicit warning | ✅ Fixed |
| `report.html` upgraded with Tailwind-style CSS + score card + footer | ✅ Fixed |
| `Dockerfile` added | ✅ Fixed |
| CI now runs `pytest` + `ruff` | ✅ Fixed |

***

## 🔴 Remaining Bugs Found in This Pass
## 🔴 One Real Bug Remaining

### Bug 1 — `stale_dags` leaks a float into the issue message
**File:** `pipelineprobe/rules/airflow_rules.py` line 92
### Bug — `report.html` uses `now()` which doesn't exist in Jinja2
**File:** `pipelineprobe/templates/report.html` line with the timestamp

```python
details=f"No successful execution in the last {days_since} days.",
```html
<span>Report generated on {{ now().strftime('%Y-%m-%d %H:%M') }}</span>
```

`days_since` is now a `float` (e.g., `7.834567...`). The message reads:
> *"No successful execution in the last 7.834567291666667 days."*

That's ugly in a user-facing report. Fix:
```python
details=f"No successful execution in the last {days_since:.1f} days.",
```

***

### Bug 2 — `check_stale_dags` misses DAGs with only failed recent runs
**File:** `pipelineprobe/rules/airflow_rules.py` lines 78–95
Jinja2 does **not** have a built-in `now()` function. This will throw `UndefinedError: 'now' is undefined` on every HTML report render, crashing the audit at the very last step. It only works if you explicitly inject `now` into the Jinja2 environment or pass it as a template variable.

The stale check only fires when `recent_successes` exists and the latest success is old. But if a DAG has run 20 times and **every single run failed**, `recent_successes` is an empty list — the `if recent_successes:` block is never entered, and the DAG silently passes the stale check even though it hasn't succeeded in months. This is the most dangerous silent miss in the whole rule set.
**Fix — Option A (simplest): pass `generated_at` from `renderer.py`**

**Fix:**
In `pipelineprobe/renderer.py`, change `render_html`:
```python
if not recent_successes:
# Has runs but zero successes — always flag as critical, not just warning
issues.append(
Issue(
severity="critical",
category="dag",
summary=f"DAG {dag.id} has no successful runs in its recent history.",
details=f"Last {len(dag.recent_runs)} runs all ended in non-success states.",
recommendation="Investigate failures immediately — this DAG has never succeeded recently.",
affected_resources=[dag.id],
)
from datetime import datetime

def render_html(self, issues: list[Issue], summary: dict) -> Path:
template = self.env.get_template("report.html")
html_content = template.render(
issues=issues,
summary=summary,
generated_at=datetime.now().strftime("%Y-%m-%d %H:%M"),
)
else:
# Has some successes — check if latest one is stale
latest_success = sorted(recent_successes, key=lambda r: r.end_time, reverse=True)[0]
...
```

***

### Bug 3 — `pyproject.toml` missing `pytest` and `pytest-mock` in dev dependencies
**File:** `pyproject.toml`

There are 6 test files but no `[project.optional-dependencies]` or `[tool.hatch.envs]` dev section. A contributor who clones and runs `pip install -e .` then `pytest` will get `ModuleNotFoundError: No module named 'pytest'`. This is a contributor experience blocker.

**Add to `pyproject.toml`:**
```toml
[project.optional-dependencies]
dev = [
"pytest>=7.4",
"pytest-mock>=3.12",
"typer[all]>=0.9",
]
Then in `report.html` update the line to:
```html
<span class="issue-category">Report generated on {{ generated_at }}</span>
```
Then document in `CONTRIBUTING.md`:
```bash
pip install -e ".[dev]"

**Fix — Option B: add `now` as a Jinja2 global in `renderer.py`**
```python
from datetime import datetime
self.env.globals["now"] = datetime.now
```
This makes `now()` available in all templates. Option A is cleaner for an audit tool since the timestamp is fixed at render time.

***

### Bug 4 — `AirflowConfig` still has a `password` hardcoded default of `"admin"`
**File:** `pipelineprobe/config.py` line 9
## 🟡 Three Things Worth Doing Before First Public Share

```python
password: str = "admin"
```
### 1. `test_rules.py` has no test for the all-failed-DAG critical path
**File:** `tests/test_rules.py`

If a user runs `pipelineprobe audit` without setting the env var and without editing the YAML, it silently attempts auth with `admin/admin`. Worse, if they're running against a real production Airflow with disabled auth, it will actually connect — and if auth is enabled with different credentials, the error message gives no hint about env vars.
`test_check_stale_dags` tests: stale success, fresh success, no runs — but **not** the new `if not recent_successes` branch that flags all-failed DAGs as critical. The commit that added this check has no corresponding test. If someone refactors that block later they won't get a regression catch.

**Fix — remove the default, make it `None` and validate:**
Add this test case to `test_rules.py`:
```python
password: str | None = None
```
Then in `AirflowConnector.__init__`, warn explicitly if password is `None`:
```python
if not self.config.password:
logger.warning(
"No Airflow password set. Set PIPELINEPROBE_AIRFLOW_PASSWORD "
"or add 'password' under orchestrator in pipelineprobe.yml"
)
def test_check_stale_dags_all_failed():
dags = [
Dag(
id="all_failed_dag",
is_active=True,
owner="test",
recent_runs=[
DagRun(state="failed", start_time=datetime.now(), end_time=datetime.now())
for _ in range(5)
],
)
]
issues = check_stale_dags({"airflow_dags": dags})
assert len(issues) == 1
assert issues[0].severity == "critical"
assert "all_failed_dag" in issues[0].affected_resources
```

***

## 🟡 Issues — Not Bugs, but Will Hurt Before PyPI Publish

### Issue 1 — `report.html` template is functional but bare — won't impress anyone
At 1.4KB, the current template is a plain HTML table with inline CSS. This is the **primary deliverable** of the entire tool — the thing you show a client or post on LinkedIn. A bare table will make the project look unpolished compared to even a modest Tailwind-styled layout.
### 2. `Dockerfile` will fail at build time — `pyproject.toml` copy without `README.md`
**File:** `Dockerfile`

**Recommended minimum upgrade for the template:**
- Add a score ring / traffic-light badge (CSS-only, no JS needed)
- Group issues by severity with collapsible sections
- Add a footer: `Generated by PipelineProbe · Built by WillowVibe`
- Add a timestamp and environment target (Airflow URL, dbt target) in the header

This is entirely a CSS/Jinja2 change — no Python changes needed. It's the highest ROI improvement left in the project.

***
```dockerfile
COPY pyproject.toml .
RUN pip install --no-cache-dir .
```

### Issue 2 — No `Dockerfile` or `docker-compose.yml` for local demo
The README likely mentions Docker but there's no `Dockerfile` in the repo root. Every data engineer's first instinct is `docker run willowvibe/pipelineprobe audit`. Without a Docker image, you lose the "zero install friction" angle that makes OSS tools go viral.
`pyproject.toml` declares `readme = "README.md"`. When pip builds the package, hatchling reads `pyproject.toml`, finds `readme = "README.md"`, tries to open it, and fails with `FileNotFoundError` because `README.md` was never copied into the image. The `RUN pip install` step will **crash the Docker build**.

**Minimal `Dockerfile`:**
**Fix:**
```dockerfile
FROM python:3.11-slim
WORKDIR /app
COPY pyproject.toml .
COPY pyproject.toml README.md ./
RUN pip install --no-cache-dir .
COPY pipelineprobe/ ./pipelineprobe/
RUN pip install -e .
ENTRYPOINT ["pipelineprobe"]
CMD ["--help"]
```

***

### Issue 3 — CI workflow likely doesn't run tests (needs verification)
The only workflow file is `ci.yml` (624 bytes — very small). A minimal CI that only lints but doesn't actually run `pytest` means the "93% test coverage" claim in the commit message is unverified on every PR. Make sure `ci.yml` includes:
```yaml
- name: Run tests
run: pip install -e ".[dev]" && pytest tests/ -v
### 3. `pyproject.toml` missing `ruff` in dev dependencies — CI will fail on a fresh clone
**File:** `pyproject.toml`

CI runs `ruff check .` but `ruff` is not listed in `[project.optional-dependencies] dev`. A contributor who does `pip install -e ".[dev]"` then runs `ruff check .` locally gets `command not found: ruff`. More critically, a fresh CI runner installing only `.[dev]` will fail at the lint step with the same error.

**Fix:**
```toml
[project.optional-dependencies]
dev = [
"pytest>=7.4",
"pytest-mock>=3.12",
"typer[all]>=0.9",
"ruff>=0.4", # ← add this
]
```

***

## Priority Order

```
Fix immediately (before any share):
1. Bug 1 — Round days_since float in stale DAG message
2. Bug 2 — Handle all-failed DAGs in stale check (critical miss)
3. Bug 3 — Add [dev] extras to pyproject.toml
4. Bug 4 — Remove hardcoded "admin" password default

Before PyPI / public announcement:
5. Issue 1 — Upgrade report.html template (it IS your product demo)
6. Issue 2 — Add Dockerfile
7. Issue 3 — Verify CI actually runs pytest
Fix immediately (runtime crash):
1. Bug — Fix now() in report.html → pass generated_at from renderer.py

Fix before sharing Docker image:
2. Issue 2 — COPY README.md in Dockerfile (build crash)

Fix before asking for contributors:
3. Issue 3 — Add ruff to dev deps in pyproject.toml
4. Issue 1 — Add test for all-failed-DAG critical branch
```

The codebase is in genuinely good shape — these are refinements, not structural problems. Fix bugs 1–4 (all small, under 30 minutes total), then focus effort on the report template. That template is your best marketing asset.
The repo is genuinely close to a clean v0.1.0 state. Fix these four items (all are small, under 20 minutes total) and you have a publishable, presentable OSS project.
7 changes: 6 additions & 1 deletion pipelineprobe/renderer.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,13 @@ def __init__(self, output_dir: str):
self.env = Environment(loader=FileSystemLoader(str(template_dir)))

def render_html(self, issues: list[Issue], summary: dict) -> Path:
from datetime import datetime

template = self.env.get_template("report.html")
html_content = template.render(issues=issues, summary=summary)
generated_at = datetime.now().strftime("%Y-%m-%d %H:%M")
html_content = template.render(
issues=issues, summary=summary, generated_at=generated_at
)
output_path = self.output_dir / "pipelineprobe-report.html"
with open(output_path, "w") as f:
f.write(html_content)
Expand Down
2 changes: 1 addition & 1 deletion pipelineprobe/templates/report.html
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ <h1>PipelineProbe</h1>
<p>Data Infrastructure Audit Engine</p>
</div>
<div class="timestamp">
<span class="issue-category">Report generated on {{ now().strftime('%Y-%m-%d %H:%M') }}</span>
<span class="issue-category">Report generated on {{ generated_at }}</span>
</div>
</header>

Expand Down
1 change: 1 addition & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ dev = [
"pytest>=7.4",
"pytest-mock>=3.12",
"typer[all]>=0.9",
"ruff>=0.4.0",
]

[project.scripts]
Expand Down
22 changes: 22 additions & 0 deletions tests/test_rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,28 @@ def test_check_stale_dags():
assert "no_runs_dag" in affected


def test_check_stale_dags_all_failed():
dags = [
Dag(
id="all_failed_dag",
is_active=True,
owner="test",
recent_runs=[
DagRun(
state="failed",
start_time=datetime.now(),
end_time=datetime.now(),
)
for _ in range(5)
],
)
]
issues = check_stale_dags({"airflow_dags": dags})
assert len(issues) == 1
assert issues[0].severity == "critical"
assert "all_failed_dag" in issues[0].affected_resources


def test_check_high_failure_rate():
dags = [
# 5 runs, 3 failed = 60% failure rate (>20%)
Expand Down
Loading