Skip to content

Sync fork with IHTSDO/master (resolves #10)#11

Open
MattCordell wants to merge 233 commits into
masterfrom
sync-ihtsdo
Open

Sync fork with IHTSDO/master (resolves #10)#11
MattCordell wants to merge 233 commits into
masterfrom
sync-ihtsdo

Conversation

@MattCordell

Copy link
Copy Markdown

Summary

Brings aehrc/master up to date with IHTSDO/release-validation-framework@master, which it had fallen ~232 commits / ~2 years behind.

The fork had diverged but with zero net custom content — its 13 unique commits are entirely self-canceling: four commit+revert pairs (M1 build helpers, an RC test-run script, "pre-requisite SQL" support, and a varchar widening attempt 4d28428 reverted by 82c1ba0) plus three older IHTSDO merge commits. The merge_base...aehrc/master net file diff was empty, so this merge applied with no conflicts and the resulting tree is identical to IHTSDO/master.

Resolves #10

Issue #10 asked to widen the description term column so full-length SNOMED terms aren't truncated. IHTSDO already shipped this in MAINT-2526 (e0ad400):

  • description_f, description_s, description_d term columns are now varchar(4096) collate utf8_bin (was varchar(256));
  • the idx_term(term) indexes were dropped — required because all tables are MyISAM (1000-byte index-key cap; varchar(4096) utf8 = 12,288 bytes would fail CREATE TABLE). This is exactly why the fork's earlier widen-only attempt (4d28428) was reverted.

Syncing therefore resolves the issue the correct way (widen and drop the index), without re-introducing the original failure. Closes #10.

Verification

  • git diff origin/master --stat (origin = IHTSDO) is empty — synced tree matches IHTSDO/master exactly.
  • All term columns in src/main/resources/sql/create-tables-mysql.sql are varchar(4096) collate utf8_bin; no idx_term remains.

Notes / follow-ups

  • The fork's previously-reverted features (pre-requisite-SQL support, M1 build helpers, RC test-run script) are not resurrected. If still wanted, re-apply them as fresh PRs on top of the synced master.
  • Downstream aehrc/rvf cleanup (per Widen description term column to support full-length SNOMED terms (varchar 4100) #10): bump the RVF image tag, delete testscripts/fix-long-terms.sh, and remove its invocation in testscripts/run-test.sh.

🤖 Generated with Claude Code

QuyenLy87 and others added 30 commits September 22, 2023 14:37
QuyenLy87 and others added 28 commits December 10, 2025 17:34
The aehrc fork was 232 commits behind IHTSDO/master with zero net custom
changes (its 13 unique commits are self-canceling commit+revert pairs plus
merge commits). This merge brings the fork up to IHTSDO/master, which includes
MAINT-2526 (description term columns widened to varchar(4096) and idx_term
indexes dropped), resolving #10.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@MattCordell

MattCordell commented Jun 15, 2026

Copy link
Copy Markdown
Author

Status / where we're up to — ⏸️ HOLD merge until after this week's production release

TL;DR: the sync itself is verified clean and mergeable, but the resulting image is not a drop-in for the existing aehrc/rvf deployment harness. Several downstream changes (entrypoint, auth model, JVM flags) would break rvf if the new image were deployed as-is. Given a production release this week, we're holding the merge until the downstream rvf adaptation is done and tested against a non-:latest test image.

✅ Verified on this PR (safe)

  • Merged tree is byte-identical to IHTSDO/master (c90f12d9^{tree} == origin/master^{tree}).
  • Fork's 13 unique commits are self-cancelling — net diff merge-base(88b15112)..aehrc/master is empty.
  • Proper 2-parent merge; history preserved.
  • Issue Widen description term column to support full-length SNOMED terms (varchar 4100) #10 resolved correctly: all term columns are varchar(4096) collate utf8_bin, idx_term removed (required under MyISAM's 1000-byte index-key cap).

Merging the PR is low-risk in isolation — it does not touch rvf. The risk is purely in deploying the new image. Do not let anything auto-deploy :latest off the back of this merge during release week.

⚠️ Downstream aehrc/rvf impact (must be done before deploying the new image)

Analysis below; rvf-side line refs are from that repo and not yet re-verified here. Framework-side items marked ✔ are confirmed against this repo.

Change Source Impact on rvf (templates/job.yaml, run-test.sh)
No Dockerfile — now built via jib-maven-plugin RAP-93 Image entrypoint is mainClass org.ihtsdo.rvf.App (no rvf-api.jar). The -jar rvf-api.jar line in job.yaml:60 will fail. New launch is jib's classpath form.
JVM flags ✔ RAP-93 / VAL-405 --add-opens=java.base/java.lang=ALL-UNNAMED is baked into the image (pom.xml), but any hand-rolled java invocation in job.yaml:60 must include it. AWS region is set via spring.cloud.aws.region.static=us-east-1 in the image; the AWS SDK v2 default-region chain may still want AWS_REGION/-Daws.region if cloud storage is enabled (not for a local useCloud=false test).
Auth model changed — SSO header, NOT Basic Auth SecurityConfig Sharper than first thought. New SecurityConfig wires only RequestHeaderAuthenticationDecorator (IHTSDO SSO header auth) with anyRequest().authenticated(); no httpBasic. run-test.sh:7-8 Basic-Auth (RVF_USER/RVF_PASSWORD) will get a JSON 401 regardless of spring.security.user.*. Fix requires either re-adding httpBasic() (fork patch to SecurityConfig) or supplying the SSO header — setting matching env vars alone is not sufficient.
Module Storage Coordinator ✔ VAL-405 / VAL-493 New module.storage.* props. Defaults are sane for a quick test (useCloud=false, local.path=store/local/, application.properties:56-62) — should work without cloud config.
rvf.empty-release-file=empty-rf2-snapshot.zip RAP-13 Confirmed at application.properties:50. Used as placeholder when no previous release is supplied; must ship in image or be mounted.
MySQL 8 PIP-521 rvf already on mysql:8.0.28 (job.yaml:41) — no change.
fix-long-terms.sh local Now redundant (schema accepts 4096, so the length >= 255 awk path never trips). Harmless to leave for first test; delete in a cleanup commit.

🧪 Building a test image locally (notes for whoever picks this up)

Target: ontoserver.azurecr.io/aehrc-rvf/release-validation-framework:sync-ihtsdo (non-:latest). Three local Windows build-env gotchas found:

  1. checkout-resources.sh fails on Windows — .gitattributes * text=auto gives it CRLF endings, breaking bash. (Linux CI is unaffected.)
  2. The script shells out to a nested mvn, which needs JAVA_HOME (only java on PATH locally).
  3. az (Azure CLI) not installed — needed to auth the ACR push.

Workaround that sidesteps 1 & 2: pre-clone the two resource repos (snomed-drools-rules, snomed-release-validation-assertions @ master) and build with -Dexec.skip=true. To push only the :sync-ihtsdo tag (and not also :8.3.1, which the pom's <tags> block would otherwise push and could overwrite a release tag in the registry):

mvn clean package jib:build -DskipTests -Pdocker-amd64 -Dexec.skip=true \
  -Dimage=ontoserver.azurecr.io/aehrc-rvf/release-validation-framework:sync-ihtsdo \
  -Djib.to.tags=

Next steps

  1. Hold this PR until after the production release.
  2. Branch rvf and adapt: job.yaml entrypoint → jib launch form; resolve the auth model change (decide: re-add httpBasic upstream-side, or switch the test harness to SSO header); confirm module-storage + empty-release defaults.
  3. Build the :sync-ihtsdo test image, point the rvf branch at it, run the pipeline end-to-end.
  4. Only then merge this PR and roll the image forward. Clean up fix-long-terms.sh separately.

See also: See also : https://github.com/aehrc/rvf/issues/25

@MattCordell MattCordell self-assigned this Jun 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Widen description term column to support full-length SNOMED terms (varchar 4100)

7 participants