Skip to content

Make dev OCI tests required#754

Open
epompeii wants to merge 1 commit intodevelfrom
u/ep/oci-url
Open

Make dev OCI tests required#754
epompeii wants to merge 1 commit intodevelfrom
u/ep/oci-url

Conversation

@epompeii
Copy link
Copy Markdown
Member

This changeset make the dev environment OCI conformance tests required.

@github-actions
Copy link
Copy Markdown

🤖 Claude Code Review

PR: #754
Base: devel
Head: u/ep/oci-url
Commit: 141401eb16789ea8e49db6d3c6d4cad8c1796949


Pull Request Review: oci_url

Summary

This PR moves the OCI conformance test from a separate CI step (in deploy.yml) into the smoke test code path, and switches OCI tests to use dedicated registry URLs instead of the API URL.

Issues

1. Orphaned CI Summary Step (Bug)
deploy.yml:93-108 — The "OCI Conformance Test Summary" step remains but the step that produces ./oci-conformance-results/test-output.log was removed. This will always print "No test output found." It should either be removed or updated to reference output from the new location (if the smoke test now produces this file).

2. Lost continue-on-error / timeout-minutes semantics
The removed CI step had continue-on-error: true and timeout-minutes: 5. Now the OCI test runs inside cargo test-api smoke dev, which has neither. If the OCI conformance test fails or hangs, it will fail the entire smoke test and block the deploy pipeline. Confirm this is the intended behavior change.

3. LOCALHOST_BENCHER_REGISTRY_URL shares port with API
Both LOCALHOST_BENCHER_API_URL and LOCALHOST_BENCHER_REGISTRY_URL resolve to localhost:61016. This is correct if the API server also serves the registry on the same port, but worth verifying — if they're separate services locally, this would cause the OCI test to hit the wrong endpoint.

4. Minor: as_registry_url takes self by copy but as_url does too — consistent, fine.

Code Quality

  • The refactoring is clean and follows existing patterns (mirroring as_url with as_registry_url).
  • Using registry_url instead of api_url for OCI tests is a good correctness fix — the SelfHosted branch at line 260 was previously passing api_url to OCI, which was wrong.
  • Import ordering follows conventions.

Recommendations

  1. Remove or update the orphaned "OCI Conformance Test Summary" step in deploy.yml.
  2. Decide on failure handling — should an OCI test failure block the deploy? If not, add error handling around oci.exec()? in the BencherCloud path (e.g., log and continue).

Model: claude-opus-4-6

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant