OpenConceptLab/ocl_issues#2838 | Pull requests run the Pylint and Tests jobs, and the Tests job really waits for Elasticsearch

[paynejd]

Closes OpenConceptLab/ocl_issues#2838. The last repo, after OpenConceptLab/oclmap#84, OpenConceptLab/oclweb3#57 and OpenConceptLab/oclweb2#52.

Summary

Pylint and the tests ran only in build.yml, on pushes to master, so a pull request showed no lint or test result before it merged.

Change

• .github/workflows/pr.yml (new): runs build.yml's "Pylint" and "Tests" jobs, unchanged, on every pull_request. That includes the Postgres 14.7 and Elasticsearch 8.15.2 service containers, the coverage run and its 95% floor. It has a read-only token, and a newer push to the same PR cancels the older run. The tests take several minutes.

• The Tests job's "Wait for Elasticsearch HTTP" step, fixed identically in build.yml and pr.yml. It never waited, for two reasons:

  • The curl URL was unquoted, so its & backgrounded curl. The rest of the line, timeout=1s >/dev/null && echo … && exit 0, then ran as a variable assignment, which succeeds, so the step passed at once every time.
  • It named the es host, which resolves only for a job running in a container. This job runs on the runner, where the service is on localhost's mapped port. The tests use ES_HOST: 0.0.0.0.

  The URL is now quoted and points at localhost:9200. The step also requires "status":"yellow" or "green" in the response body. Since Elasticsearch 8.0, a wait_for_status that times out still answers 200, with "timed_out": true (found by Codex, fixed in ad20cc9). Anything else is another try: a refused connection, a non-2xx, or a red or timed-out body. The loop is unchanged: up to 60 tries, 2 s apart, then the job fails.

build.yml's build and deploy jobs are untouched.

Test plan

• pr.yml's two jobs are identical to build.yml's Pylint and Tests jobs (diffed after the fix).

• CI's lint, pylint -j0 core/, in the local oclapi2 test image (Python 3.12): 10.00/10.

• The fixed wait step parses as bash.

• The new "PR checks" jobs ran on this PR and passed, on 6c36c35 and again on ad20cc9: Pylint, and Tests (2310 tests, 95% coverage).

• The Elasticsearch wait works. On ad20cc9, the step reached localhost:9200, found yellow or green, and printed "Elasticsearch is ready!" 75 ms after it started. ES had had about 3 minutes to start while dependencies installed.

• It really waits. Run locally under bash -e, which is how GitHub runs this step, and under bash -eo pipefail:

  • Old line, nothing listening: printed "Elasticsearch is ready!"
  • New loop, nothing listening, or a red/timed-out 200 body: "Still not ready..." on each try, then fails.
  • New loop, yellow or green body: ready at once.

• Codex adversarial review, posted on this PR:

  • Pass 1 (6c36c35): one P2, fixed in ad20cc9. ES 8 answers 200 on a wait_for_status timeout. Codex otherwise confirmed the jobs, services and env match build.yml.
  • Pass 2 (ad20cc9): clean.

  A read-only Claude review, posted earlier as a stand-in, was also clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Lkm1wDY1zKSDTm2apzPJLi

[paynejd]

Claude review subagent: stand-in until the Codex pass runs (commit 6c36c35)

Codex was at its usage limit, so a read-only Claude Code review subagent reviewed this PR first. It looked specifically at the Elasticsearch wait fix. The Codex adversarial review will be posted separately, and the PR waits for it before merging.

Outcome: No issues found.

---

No real issues found. The change is correct as written.

The Elasticsearch wait fix

• localhost:9200 reaches ES. The job runs on the runner host, and ports: - 9200:9200 publishes the port there, so the request gets to the container through Docker's port mapping. es only resolves for container jobs, so the old host was wrong. A request that arrives before ES is listening fails fast with a reset or empty reply. It does not hang.
• -f and the query string behave as intended.
  • Connection refused exits 7.
  • A 503 or a 408 on timeout=1s exits 22.
  • Only a 200 at yellow or green passes.
  • With no indices, a single node reports green right after it elects itself master.
• The && list is safe under bash -e. A failing command that isn't last in an && list doesn't trigger errexit. I checked locally: false && echo && exit 0 inside the loop kept iterating and reached the final line. So the explicit exit 1 after the loop is the only way the step fails.
• Budget. The loop gives at least 60 × 2 s ≈ 120 s, and that clock only starts after checkout, Python setup and pip install, while ES has been booting since container init. ES 8.15 in single-node mode starts in well under that. If ES never comes up, the old step passed silently and the job then failed at search_index --create. Failing earlier with a clear message is the better trade.

Everything else

• Jobs match. A diff of pr.yml:18-135 against build.yml:22-139 shows only a trailing blank line.
• Safe for fork PRs.
  • It triggers on pull_request, not pull_request_target.
  • It sets permissions: contents: read and references no secrets.
  • It has no push, release or deploy steps.
  • Cache writes are scoped to the PR ref.
• Concurrency is independent. pr.yml uses pr-checks-<number> and build.yml uses release-<ref>, so they can't collide.
• YAML and whitespace are clean. Ruby's YAML parser loads both files, git diff --check is clean, and pr.yml ends with a newline.

Non-blocking note (not a defect): the Pylint and Tests jobs are copied rather than shared, so the two files can drift apart. The header comment at pr.yml:3-5 says to keep them the same. A reusable workflow would enforce that, but it would mean restructuring build.yml, which the ticket keeps out of scope.

[paynejd]

Codex adversarial review, pass 1 (codex-cli 0.158, commit 6c36c35)

Scope: origin/master...HEAD, with a close look at the Elasticsearch wait and at whether pr.yml's Tests job (services, env, steps) matches build.yml's. Read-only run. The report below is verbatim, except that local file links point to this commit on GitHub.

Outcome: One P2, fixed in ad20cc9. Since ES 8 a timed-out wait_for_status answers 200. The step now requires "status":"yellow" or "green" in the body, and retries otherwise. Checked under bash -eo pipefail with a red/timed-out body (retries, then fails), yellow and green bodies (ready), and a refused connection (retries, then fails).

---

One real issue:

[P2] Elasticsearch readiness can still pass before yellow — .github/workflows/build.yml:100 and .github/workflows/pr.yml:96.

Elasticsearch 8.15.2 returns HTTP 200, not 408, when wait_for_status times out, with "timed_out": true in the JSON. This changed in Elasticsearch 8.0. Consequently, if HTTP is available but the cluster remains red after one second, curl -sf succeeds and the step prints “Elasticsearch is ready!” and exits immediately. The remaining retries are bypassed, allowing subsequent operations against an unready cluster. Check the response’s timed_out field and yellow/green status before exiting successfully. Elastic’s migration documentation.

No other issues found. Parsed Pylint and Tests jobs match exactly, including all services, environments, commands, and needs. YAML and Bash syntax checks passed; simulated failures confirmed bash -e preserves retries. No files modified.

[paynejd]

Codex adversarial review, pass 2 (codex-cli 0.158, commit ad20cc9)

Scope: origin/master...HEAD, focused on the ES status check that pass 1 prompted, under bash --noprofile --norc -eo pipefail. Read-only run. The report below is verbatim.

Outcome: Clean.

---

No real issues found. Verified retries and success paths under bash --noprofile --norc -eo pipefail, including red/timed-out responses. The match handles ES’s compact JSON regardless of field order. Tests jobs are identical in both workflows. No files modified.