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
Rundeck Access and others added 27 commits January 5, 2026 10:02
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
dionmcm added a commit that referenced this pull request Sep 2, 2026
…e work

snomed-parent-bom 4.0.2 resolves snomed-drools-engine to 6.0.0, and tag 6.0.0
is three commits BEHIND the first of the six performance PRs SI merged into
IHTSDO/snomed-drools develop on 2026-08-28. No tag has been published since,
so nothing in the BOM's reach contains them:

  #6  keep workers supplied with concepts instead of batches of ten
  #7  scale validation workers to the host, callers can override
  #8  skip the effective-component pre-pass for a single Snapshot
  #9  read the RF2 files concurrently, repository made safe for it
  #10 exact-term description lookups from a map, not a Lucene index
  #11 compile the whitespace split pattern once

Measured on the AU edition, 722,404 concepts, this host: Drools total run
time 697 s on 6.0.0. The recorded figure with this work applied is 54 s.

duck/build-drools-engine.sh builds IHTSDO's own 84d511b - the merge commit of
#11, the last of the six, and the last point still on snomed-parent-bom 4.0.0.
develop has since moved to 7.0.0-SNAPSHOT against snomed-parent-bom
5.0.0-SNAPSHOT, which RVF does not build against and which is unreleased, so
84d511b keeps the delta to exactly the six PRs.

The only edit the script makes is the version string, and that is needed
because SI's commit is internally inconsistent: "Start 6.1.0-SNAPSHOT" bumped
the root pom and left both submodules on parent 6.0.0.

Two things the script refuses to build past, because "I built the wrong
commit" is exactly the mistake that would go unnoticed:

  * every one of the six must be an ancestor of HEAD, checked against the
    GitHub API
  * the bytecode must show the change. 6.0.0 batches ten with a `bipush 10`
    and never calls availableProcessors; the built jar must be the reverse.
    Verified: 6.0.0 has 1 and 0, the built jar has 0 and 1.

It also pre-fetches two plexus artifacts and their parent poms from Central,
because maven-install-plugin 2.4 needs them and offline resolution goes to
nexus3.ihtsdotools.org, which hangs rather than 404s - 25 minutes and zero
bytes before I killed it.

DROP the pin and delete the script when SI publishes a release.
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