ci: unblock Oracle in compat matrix and fix Lucee 6 datasource#2683
Conversation
bpamiri
force-pushed
the
peter/fix-2663-oracle-ci
branch
from
4d6b11d to
3273b99
Compare
![wheels-bot[bot]](https://avatars.githubusercontent.com/in/3657588?s=60&v=4){kind=small}
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This is a tight CI infrastructure PR fixing two independent Oracle problems: Lucee 6 datasource registration (wrong dbdriver value) and Oracle soft-failing in the compat matrix while sub-cluster 3 is addressed in a follow-up. The diff is correct and proportionate. I am posting as comment rather than request-changes because all findings are minor — no correctness, cross-engine, or security issues apply to CI-infra-only changes of this shape.
Correctness
Lucee 7 still uses "dbdriver": "Oracle" — worth one sentence of explanation.
tools/docker/lucee7/CFConfig.json:148
The fix switches Lucee 6 to "Other" because Lucee 6's Oracle extension re-templates the JDBC URL. Lucee 7 keeps "Oracle" and apparently works. The reasoning is clear from issue #2663 (sub-cluster 1 is Lucee-6-only per the CI evidence), but neither the commit message nor the PR body states why Lucee 7 is exempt. A one-line comment in CFConfig.json or a sentence in the commit body would prevent the next person from asking the same question and applying the same fix to Lucee 7 unnecessarily.
Suggested addition to PR description:
Lucee 7 retains
"dbdriver": "Oracle"— its Oracle extension does not re-template the JDBC URL, so it connects successfully with the current config. Only Lucee 6 exhibited theInvalid Oracle URL specifiedfailure.
Matrix run is unchecked in the test plan.
The primary claim of this PR — that dbdriver: "Other" allows Lucee 6 to register the Oracle datasource — cannot be verified without a real Oracle matrix run. The [ ] Trigger compat-matrix on this branch checkbox is intentionally left open. The evidence that "Other" is correct (Adobe 2023/2025 and BoxLang configs use "Other" and connect successfully) is reasonable indirect justification, but confirming that checkbox before merge would close the loop if Oracle infra is available.
Conventions
SOFT_FAIL_DBS is assigned inside the per-database loop at line 422 but outside the loop at line 470.
At line 422, SOFT_FAIL_DBS="oracle" sits inside the for db in "${DBS[@]}" loop that runs from line 285 to 446 — it is reassigned on every database iteration. At line 470 the same variable is set once before the loop, which is the cleaner pattern. Both are functionally correct since the value is constant, but the inconsistency is worth cleaning up: move the line-422 assignment to just before the for db in at line 285 to match line-470 style.
Docs
PR title is stale.
The PR title reads: ci: unblock Oracle in compat matrix and guard advisory-lock spec
The PR body explicitly says the advisory-lock guard (sub-cluster 2, already landed via #2670) has been dropped from this PR. The commit message correctly says fix Lucee 6 datasource. If this squash-merges, the merge commit will use the PR title and will be misleading. Suggest updating to:
ci: unblock Oracle in compat matrix and fix Lucee 6 datasource
CLAUDE.md ~ line numbers are a good call.
Previously the doc cited exact lines 389 and 519. Switching to ~421, ~469 (actual: 422 and 470) is more honest and less likely to rot. The one-line-off approximation is fine under the tilde.
Commits
Single commit, conforming to commitlint:
- Type:
ci✓ - Subject within 100 chars ✓
- Not ALL-CAPS ✓
- Body explains the "why" clearly ✓
Note: (#2663) in the subject references the issue, not the PR (#2683). Not a rule violation, just worth being aware of if the project convention is PR-number references.
Match the pattern already used in the summary step (line 470) where SOFT_FAIL_DBS is set once before the loop. The previous placement reassigned the same constant on every database iteration. Same value, cleaner scope. Per PR #2683 review feedback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bpamiri
changed the title
|
Thanks for the review. Addressed:
|
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This follow-up commit correctly addresses the previous review's finding — SOFT_FAIL_DBS is now set once before the per-database loop at line 275 with a clear comment, rather than being reassigned on every iteration. The diff is proportionate and the hoist is functionally correct. I am requesting changes for two issues: both commits are missing the required DCO Signed-off-by: trailer (a required CI status check that will block merge), and the CLAUDE.md line reference introduced in the first commit is now stale after the second commit moved the first occurrence from ~421 to 275.
Commits
DCO sign-off missing on both commits.
Both commits in this PR are missing the required Signed-off-by: trailer:
3273b99a5—ci: unblock Oracle in compat matrix and fix Lucee 6 datasource (#2663)f845ffa1c—ci(compat-matrix): hoist SOFT_FAIL_DBS out of per-db loop
Each ends with:
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
but no Signed-off-by: Peter Amiri <peter@alurium.com> trailer. Per CONTRIBUTING.md § DCO:
The DCO GitHub App is a required status check on every PR and will block merges if any commit is missing the trailer.
Fix: add Signed-off-by: to both commits (e.g. amend locally and force-push, or ask a maintainer to apply the trailer on merge if the project permits that).
Docs
CLAUDE.md line reference is stale after the hoist.
CLAUDE.md line 642 (introduced in the first commit) reads:
`SOFT_FAIL_DBS` in `.github/workflows/compat-matrix.yml` (lines ~421, ~469) currently contains `"oracle"`
After the second commit hoisted SOFT_FAIL_DBS from inside the per-db loop to before it, the actual line numbers are now 275 and 470. The ~ approximation cannot bridge a 146-line gap — ~421 will send a reader to the wrong place. Suggest updating to lines ~275, ~470 or, more durably, replacing the line numbers with prose:
`SOFT_FAIL_DBS` appears twice in `.github/workflows/compat-matrix.yml`: once before the per-database test loop, and once before the per-database summary loop.
Minor nit (non-blocking)
Second SOFT_FAIL_DBS assignment at line 470 is now redundant.
Both assignments are in the same run: block, so the variable set at line 275 persists to the summary loop at line 470. The second assignment is harmless and adds a useful "close to the consumer" reminder if the step is ever split. Worth a one-line comment or no change at all.
Two of the three sub-clusters from #2663 — the third (lockingSpec guard, #2665) already landed via #2670 on develop, so this rebase drops the manual hard-coded skip list in favor of the merged $supportsAdvisoryLocks() capability. 1. Lucee 6 datasource registration: switch dbdriver from "Oracle" to "Other" in tools/docker/lucee6/CFConfig.json so the JDBC URL is used as-is rather than re-templated by Lucee's Oracle extension. Matches the working Adobe 2023/2025 and BoxLang configs. 2. Add oracle to SOFT_FAIL_DBS in compat-matrix.yml so the remaining Oracle constraint-cleanup issues (ORA-02264 / ORA-01003) log as warnings instead of breaking each engine job while a follow-up resolves the migratorSpec / bulkOperationsSpec teardown drift. Constraint cleanup (sub-cluster 3 in the issue) is left for a separate PR — shares shape with #2664's MySQL/H2/SQL Server FK collision work. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com>
Match the pattern already used in the summary step (line 470) where SOFT_FAIL_DBS is set once before the loop. The previous placement reassigned the same constant on every database iteration. Same value, cleaner scope. Per PR #2683 review feedback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com>
Line numbers shift when assignments move (as in the previous commit which hoisted SOFT_FAIL_DBS out of the per-db loop). Replace the `~421, ~469` references with prose tied to the workflow step names so the docs stop rotting on every adjacent edit. Per PR #2683 review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com>
bpamiri
force-pushed
the
peter/fix-2663-oracle-ci
branch
from
f845ffa to
3ae22fa
Compare
|
Addressed both blocking items:
On the nit (second |
Wheels Bot — Reviewer B (round 1)A's review is mostly accurate: the DCO finding and the stale CLAUDE.md line-number finding are both legitimate. However, the "minor nit" about the second SycophancyNone detected. A requested changes and cited specific evidence for each finding. False positives
Missed issues
Verdict alignmentA's CHANGES_REQUESTED verdict is correct given the real DCO issue and the stale CLAUDE.md line numbers; I agree with it. ConvergenceThe DCO and CLAUDE.md findings are real and unresolved. The false-positive "redundant assignment" nit is harmless only because A labelled it non-blocking — the author should be explicitly told not to remove the line 470 assignment. Once (1) both commits are amended or rebased to add |
|
Reviewer B — your analysis is correct on all three points (DCO + stale CLAUDE.md were real and unresolved at
Your timing crossed with my response to A which pushed back on the redundant-assignment nit using the same reasoning you arrived at independently. Convergence confirmed. |
Wheels Bot - Reviewer B (round 1)A's review is accurate and proportionate. All claims verified. No sycophancy, no false positives, no missed issues found on re-scan. Convergence on the changes A flagged. Sycophancy: None. A avoids approval and backs correctness claims with specific findings. False positives: None. Verified all three: Lucee 7 does retain dbdriver Oracle at CFConfig.json:148. SOFT_FAIL_DBS at ~line 422 is inside the for-db loop - the done keyword closes 25 lines later, so A's inconsistency finding is accurate (functionally harmless, style gap real). PR title does retain 'guard advisory-lock spec' which the PR body says was dropped on rebase. Missed issues: None significant. DCO Signed-off-by trailer is absent from the commit, but the DCO GitHub App check is not installed in this repo's CI (confirmed by inspecting check-runs on the SHA), so not a blocker. No cross-engine CFML concerns apply - changed files are JSON config and a shell workflow. Verdict alignment: COMMENT is consistent with all findings being style/documentation with no correctness impact. Convergence: Aligned with A's COMMENT verdict. Three concrete actionable findings to address: (1) update PR title to drop the stale advisory-lock reference, (2) move the SOFT_FAIL_DBS assignment before the for-db loop at ~line 285 to match the second location's style, (3) add a brief note in the PR description or config explaining why Lucee 7 retains dbdriver Oracle while Lucee 6 requires Other. |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This PR has fully addressed both prior Reviewer A rounds. All three commits carry Signed-off-by trailers (the DCO blocker from round 2), SOFT_FAIL_DBS is now set once before each loop rather than inside the per-database iteration, and CLAUDE.md documents the two assignment sites by workflow step name rather than line numbers. The diff is clean. I am approving.
Docs
Minor nit (non-blocking): CLAUDE.md step name is slightly truncated.
CLAUDE.md line 642 (post-commit) reads:
once in the "Run test suites" step (before the per-database loop)
The actual step name in compat-matrix.yml line 260 is:
- name: Run test suites for all databasesThe truncation is unambiguous in context, but a future maintainer doing a literal grep for the step name won't find a match. Consider using the full name, or the step ID (id: run-tests) which is shorter and won't silently diverge on a rename. Non-blocking — the intent is clear.
Commits
All three commits conform to commitlint and the DCO requirement:
| SHA | Subject | Type | DCO |
|---|---|---|---|
906172b |
ci: unblock Oracle in compat matrix and fix Lucee 6 datasource (#2663) |
ci ok |
Signed-off-by ok |
fcc1c65 |
ci(compat-matrix): hoist SOFT_FAIL_DBS out of per-db loop |
ci ok |
Signed-off-by ok |
3ae22fa |
docs(claude): describe SOFT_FAIL_DBS locations by step, not line number |
docs ok |
Signed-off-by ok |
Subjects within 100 chars, not ALL-CAPS, bodies explain the "why." The DCO blocker from the previous round is fully resolved.
Correctness
The SOFT_FAIL_DBS="oracle" assignment now appears exactly twice — once at the top of the "Run test suites for all databases" run: block (~line 275) and once at the top of the "Generate per-engine summary" run: block (~line 470). Because each GitHub Actions run: step launches a fresh shell, the two assignments must be kept in sync; both are, and CLAUDE.md now documents this constraint explicitly. The grep -qw "$db" word-boundary check is correct for the current single-value case and would continue to work if additional database names were added space- or comma-separated.
tools/docker/lucee6/CFConfig.json: the dbdriver change from "Oracle" to "Other" aligns Lucee 6 with the Adobe 2023/2025 and BoxLang configs that already use "Other" and connect successfully. Lucee 7 intentionally retains "dbdriver": "Oracle" — the PR body explains the Lucee-6-specific URL re-templating failure, and the asymmetry is correct.
The unchecked test-plan item ([ ] Trigger compat-matrix on this branch) is noted; the SOFT_FAIL_DBS safety net means any remaining Oracle failures will log as warnings rather than blocking the build, which appropriately limits the blast radius while the follow-up (sub-cluster 3) lands.
|
Reviewer B round 2 — your review is anchored at SHA
Also useful: your note that the DCO GitHub App is not installed in this repo's CI. I went ahead and added The PR is ready for re-review at |
Wheels Bot — Address Review (round 1)Both consensus findings from Reviewer A and Reviewer B's round-1 convergence (at Consensus findings (from B's
Skipped (not in consensus):
Because no commit is pushed by this run, no fresh Reviewer A will fire from address-review. The convergence loop should next run Reviewer A against |
Wheels Bot — Reviewer B (round 1)A's review is accurate and proportionate on the current HEAD. All claims verified against the diff and git log. No sycophancy, no false positives, no missed issues. Aligned on approve. SycophancyNone detected. A backed the APPROVE with specific verification: DCO trailers confirmed on all three commits, commit subjects checked against commitlint, SOFT_FAIL_DBS placement verified at both step boundaries, CFConfig.json change traced to the working Adobe/BoxLang precedent, and the unchecked test-plan item explicitly acknowledged with a reasoned rationale (soft-fail safety net limits blast radius). Not a rubber stamp. False positivesNone detected. Verified all A's claims directly:
Missed issuesNone detected. The diff touches only CI YAML, a JSON config file, and CLAUDE.md — no CFML, no cross-engine concerns to probe. The unchecked Oracle matrix run is a known gap, and the SOFT_FAIL_DBS design is explicitly the safety net for it. Verdict alignmentAPPROVE is consistent with A's findings. All prior blockers (DCO, SOFT_FAIL_DBS placement, stale CLAUDE.md) are resolved at this SHA. The one remaining nit (truncated step name) is non-blocking by A's own assessment, which I agree with. ConvergenceAligned. A's APPROVE is the right call. The PR is review-clean on this SHA. |
Match the workflow step name verbatim per PR #2683 reviewer nit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The head commit bb4d08c9 directly addresses the sole nit from the previous Reviewer A approval (round 3) — CLAUDE.md now references the exact step name "Run test suites for all databases" rather than the truncated version. Both step names in the docs match the workflow verbatim (confirmed: line 260 Run test suites for all databases, line 462 Generate per-engine summary). The PR is clean and I am approving.
Correctness
All substantive changes were verified in prior rounds and are unchanged:
- Lucee 6
dbdriverfix (tools/docker/lucee6/CFConfig.json): switching from"Oracle"to"Other"prevents the Lucee Oracle extension from re-templating the JDBC URL, matching the working Adobe 2023/2025 and BoxLang configs. Lucee 7 correctly retains"Oracle". SOFT_FAIL_DBShoist (compat-matrix.ymlline 275): set once before the per-database loop rather than reassigned on every iteration. Bothrun:steps keep their own assignment because GitHub Actions launches a fresh shell per step — this is correct and now documented explicitly in CLAUDE.md.- CLAUDE.md update: step names are now exact matches (
"Run test suites for all databases","Generate per-engine summary"), making grep-based navigation reliable and eliminating the line-number rot problem.
Commits
All four commits conform to commitlint and carry DCO sign-offs:
| SHA | Subject | Type | DCO |
|---|---|---|---|
906172b |
ci: unblock Oracle in compat matrix and fix Lucee 6 datasource (#2663) |
ci ok |
Signed-off-by ok |
fcc1c65 |
ci(compat-matrix): hoist SOFT_FAIL_DBS out of per-db loop |
ci ok |
Signed-off-by ok |
3ae22fa |
docs(claude): describe SOFT_FAIL_DBS locations by step, not line number |
docs ok |
Signed-off-by ok |
bb4d08c |
docs(claude): use exact step name "Run test suites for all databases" |
docs ok |
Signed-off-by ok |
Subjects within 100 chars, not ALL-CAPS, bodies explain the "why". The DCO blocker from round 2 was resolved in round 3 and holds here.
Remaining open item (non-blocking)
The test-plan checkbox [ ] Trigger compat-matrix on this branch is still open. With SOFT_FAIL_DBS="oracle" in place any residual Oracle failures will log as warnings rather than block merge, which is the appropriate safety net while sub-cluster 3 lands separately. This is understood and acceptable.
Wheels Bot — Reviewer B (round 1)A's review is accurate and proportionate on the current HEAD. All claims verified. No sycophancy, no false positives, no missed issues. Aligned on approve. SycophancyNone detected. A backed the APPROVE with specific verifications: DCO trailers confirmed on all four commits, CLAUDE.md step names checked verbatim against the workflow, and the unchecked test-plan item explicitly acknowledged with a clear rationale (SOFT_FAIL_DBS limits blast radius). False positivesNone detected. Verified A's key claims directly:
Missed issuesNone detected. The diff touches only CI YAML, a JSON config file, and CLAUDE.md — no CFML, no cross-engine concerns. The Verdict alignmentAPPROVE is consistent with A's findings. All prior blockers (DCO, SOFT_FAIL_DBS placement, stale CLAUDE.md) are resolved. The sole remaining nit from round 3 (truncated step name) is confirmed fixed in ConvergenceAligned. A's APPROVE is the right call. The PR is review-clean on this SHA. |
… to avoid FK collision Closes #2663 (sub-cluster 3) and resolves #2664. The migrator spec's migrateTo describe block was leaking the wheels_migrator_versions / c_o_r_e_migrator_versions tables (plus their fk_wheels_level FK constraint) between tests. When the "uses specified versions table name" test then swapped migratorTableName to c_o_r_e_migrator_versions, the bootstrap path in $getVersionsPreviouslyMigrated() added the same fk_wheels_level constraint to a different table — MySQL, H2, SQL Server, and Oracle scope FK names per-schema rather than per-table, so the second CREATE failed with ORA-02264 ("name already used by an existing constraint") / "Duplicate foreign key constraint name". Mirror the cleanup pattern already used by the F15 Phase 1 describe block: in beforeEach, drop both table-name families inside try/catch so the bootstrap recreates them cleanly per test. wheels_levels can stay — it is referenced by the FK, not the source of it, and the bootstrap is happy to reuse it. This completes the three-cluster fix for #2663: - sub-cluster 1 (Lucee 6 Oracle JDBC URL) landed in #2683. - sub-cluster 2 (DBMS_LOCK / lockingSpec guard) landed via #2670. - sub-cluster 3 (constraint cleanup) — this PR. Removing "oracle" from SOFT_FAIL_DBS in compat-matrix.yml is left for a follow-up once a green Oracle matrix run is observed. Signed-off-by: Claude <noreply@anthropic.com>
1 participant
Summary
Two of the three sub-clusters from #2663. Sub-cluster 2 (lockingSpec guard) already landed on develop via #2670 — its
$supportsAdvisoryLocks()capability supersedes the manual skip list this PR originally proposed, and that change has been dropped on rebase.Lucee 6 datasource registration — Switch
dbdriverfrom"Oracle"to"Other"in tools/docker/lucee6/CFConfig.json. The Lucee Oracle extension was re-templating the JDBC URL and emittingjava.sql.SQLException: Invalid Oracle URL specified. Adobe 2023/2025 and BoxLang already use"Other"and work fine; this is the minimum diff between the working configs and Lucee 6.Lucee 7 retains
"dbdriver": "Oracle"— its Oracle extension does not re-template the JDBC URL, so it connects successfully with the current config. TheInvalid Oracle URL specifiedfailure in the compat matrix is Lucee-6-only (per the issue's CI evidence). Leaving Lucee 7 alone avoids risking a regression on a path that works today.SOFT_FAIL_DBS="oracle"— Set once before the per-database loop and read inside it. Oracle's remaining constraint-cleanup failures (ORA-02264 / ORA-01003 inmigratorSpec/bulkOperationsSpec) now log as warnings instead of breaking each engine job while a follow-up addresses the teardown drift.CLAUDE.md's
SOFT_FAIL_DBSsection is updated to reflect the new state.Out of scope (follow-up)
Sub-cluster 3 from the issue — Oracle ORA-02264 constraint cleanup in
migratorSpec/bulkOperationsSpec— needs the same shape of fix as #2664's MySQL/H2/SQL Server FK collision work. Leaving it for a separate PR keeps each change reviewable on its own.Test plan
bash tools/test-local.sh model— 815 pass on Lucee 7 + SQLite (verified pre-rebase)Refs: #2649 (rollup), #2670 (lockingSpec already merged), #2664 (FK constraint cleanup, related shape)
🤖 Generated with Claude Code