fix(ibis): share Oracle instance when v3 testing#1371
fix(ibis): share Oracle instance when v3 testing#1371goldmedal wants to merge 4 commits intoCanner:mainfrom
Conversation
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ibis-server/tests/routers/v3/connector/oracle/conftest.py (1)
46-47: Maintain compatibility with pre-23c Oracle health check.
SELECT 1without aFROMclause only works on Oracle 23c and newer; earlier releases throwORA-00923and this retry loop would exhaust all attempts if someone runs the tests against an older image. The safer option is to keep the traditionalFROM dual, which still works on 23c+.(docs.oracle.com)- result = conn.execute(text("SELECT 1")) + result = conn.execute(text("SELECT 1 FROM dual"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ibis-server/tests/routers/v3/connector/oracle/conftest.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ibis-server/tests/routers/v3/connector/oracle/conftest.py (1)
ibis-server/tests/routers/v2/connector/test_oracle.py (1)
oracle(119-171)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ibis-ci-2.yml (1)
63-66: Recommend adding observability for Oracle test execution.The test step only sets
WREN_ENGINE_ENDPOINTbut does not expose Oracle-specific diagnostics or error handling. Consider:
- Adding timeout configuration for Oracle tests (via pytest timeout plugin or custom markers)
- Logging retry attempts from test fixtures for visibility into connectivity issues
- Adding conditional reporting or artifact uploads on test failure to aid debugging
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ibis-ci-2.yml(1 hunks).github/workflows/ibis-ci.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.8)
.github/workflows/ibis-ci-2.yml
51-51: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ci
- GitHub Check: ci
🔇 Additional comments (2)
.github/workflows/ibis-ci.yml (1)
77-77: Marker filter correctly excludes oracle tests from main CI.The addition of
not oracleto the pytest marker filter aligns with the new dedicated Oracle CI workflow. This ensures oracle-specific tests run in an isolated environment (ibis-ci-2.yml) rather than in the standard CI pipeline..github/workflows/ibis-ci-2.yml (1)
1-66: Review comment is incorrect — Oracle infrastructure is properly configured.The workflow correctly provisions Oracle via testcontainers, not system-level drivers. The V3 oracle conftest.py imports
OracleDbContainerwhich dynamically starts a Docker-based Oracle instance (image: gvenzl/oracle-free:23.6-slim-faststart) with robust retry logic:
- Container provisioning: Testcontainers starts Oracle on test run (fixture scope="session")
- Connection retry: 30 retries with 10-second intervals (300s total timeout) including health checks (
SELECT 1)- Credentials & environment: Connection URL, host, and port extracted automatically from container; no manual env var setup needed
- Dependency present: testcontainers 4.9.2 with oracle extra in pyproject.toml
The MS ODBC installation in ibis-ci.yml is for SQL Server tests (v2), while ibis-ci-2.yml uses testcontainers for Oracle (v3)—different architectural approaches, both correct.
The workflow's
-m "oracle"pytest marker correctly filters oracle-tagged tests. No issues found.Likely an incorrect or invalid review comment.
| - name: Cache Cargo | ||
| uses: actions/cache@v3 | ||
| with: | ||
| path: | | ||
| ~/.cargo/bin/ | ||
| ~/.cargo/registry/index/ | ||
| ~/.cargo/registry/cache/ | ||
| ~/.cargo/git/db/ | ||
| wren-core-py/target/ | ||
| key: ${{ runner.os }}-cargo-${{ hashFiles('wren-core-py/Cargo.lock') }} |
There was a problem hiding this comment.
Update actions/cache to v4 to address outdated action warning.
The workflow uses actions/cache@v3, which is flagged as outdated by actionlint. Update to v4 for improved security, bug fixes, and continued support.
Apply this diff to update the cache action:
- - name: Cache Cargo
- uses: actions/cache@v3
+ - name: Cache Cargo
+ uses: actions/cache@v4📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Cache Cargo | |
| uses: actions/cache@v3 | |
| with: | |
| path: | | |
| ~/.cargo/bin/ | |
| ~/.cargo/registry/index/ | |
| ~/.cargo/registry/cache/ | |
| ~/.cargo/git/db/ | |
| wren-core-py/target/ | |
| key: ${{ runner.os }}-cargo-${{ hashFiles('wren-core-py/Cargo.lock') }} | |
| - name: Cache Cargo | |
| uses: actions/cache@v4 | |
| with: | |
| path: | | |
| ~/.cargo/bin/ | |
| ~/.cargo/registry/index/ | |
| ~/.cargo/registry/cache/ | |
| ~/.cargo/git/db/ | |
| wren-core-py/target/ | |
| key: ${{ runner.os }}-cargo-${{ hashFiles('wren-core-py/Cargo.lock') }} |
🧰 Tools
🪛 actionlint (1.7.8)
51-51: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
.github/workflows/ibis-ci-2.yml around lines 50 to 59: the workflow uses
actions/cache@v3 which is flagged as outdated; update the action reference from
actions/cache@v3 to actions/cache@v4 in the Cache Cargo job (keep the existing
with: keys and paths unchanged) so the workflow uses the current major version
and removes the actionlint warning.
1 participant
Summary by CodeRabbit