feat(deployment,website): Split website organism configs into separate ConfigMaps#6447
feat(deployment,website): Split website organism configs into separate ConfigMaps#6447theosanderson-agent wants to merge 9 commits into
Conversation
|
Claude finished @theosanderson's task in 3m 35s —— View job Code Review
Overall this is a clean, well-motivated change. The size numbers in the PR description make the need obvious, the Helm splitting logic is straightforward, and the website-side merge is simple and well-tested. A few small things to tighten up: Inline comments posted:
Nothing blocking; the core approach is sound and the test for split-config merging in |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 962bffe521
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@corneliusroemer @maverbiest @tombch would one of you have a chance to review? I think this is quite important as I think it could currently be blocking adding any more organisms |
|
@theosanderson I might have some time over the weekend/early next week so will do my best to review then |
|
|
||
| /website/tests/config/*.json | ||
| /website/tests/config/*.yaml | ||
|
|
There was a problem hiding this comment.
Feel free to skip as this is not an issue but I guess these could be moved into the websites .gitignore?
| {{- end }} | ||
|
|
||
| {{- define "loculus.websiteOrganismConfigMapName" -}} | ||
| {{- printf "loculus-web-org-config-%s" . | trunc 63 | trimSuffix "-" -}} |
There was a problem hiding this comment.
Is there a reason why the truncation limits to 63 characters, rather than the limit for config map names (253 characters)?
There was a problem hiding this comment.
Ah, I have had a look around and organism names are used in a bunch of labels which limit to 63 characters, so I guess this is already a thing that needs restricting to lower than 253 chars (as mentioned in the resolved comment, this is already an issue)
tombch
left a comment
There was a problem hiding this comment.
Tested it locally as well, looks good to me!
Summary
Splits the website organism configuration out of the monolithic
loculus-website-configConfigMap into one ConfigMap per organism. The website now readswebsite_config.jsonplus optionalorganisms/*.jsonfiles and merges them at startup.This keeps generated website organism config data out of a single Kubernetes object so instances with many or large organisms stay comfortably below the ConfigMap/Secret object size limit.
Implementation
loculus-website-configwith global website config only and oneloculus-web-org-config-*ConfigMap per organism.organisms/*.jsonlands under the sameCONFIG_DIR.deploy.py configto generate split local test config files.Size check
Using current Pathoplexus values with the Bundibugyo organism branch:
loculus-website-config: 976,493 bytes ofwebsite_config.jsonloculus-website-config: 2,568 bytes ofwebsite_config.jsonin the main renderloculus-web-org-config-mpox)loculus-backend-config), about 11.75% of 1 MiBThe reconstructed split website config matches the old monolithic website config for the Pathoplexus main render.
🚀 Preview: Add
previewlabel to enable