Skip to content

Not to be merged: testing new file ids on preview - #7306

Draft
maverbiest wants to merge 11 commits into
mainfrom
test-new-file-ids
Draft

Not to be merged: testing new file ids on preview#7306
maverbiest wants to merge 11 commits into
mainfrom
test-new-file-ids

Conversation

@maverbiest

@maverbiest maverbiest commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

resolves #

Screenshot

PR Checklist

  • All necessary documentation has been adapted.
  • The implemented feature is covered by appropriate, automated tests.
  • Any manual testing that has been done is documented (i.e. what exactly was tested?)

🚀 Preview: https://test-new-file-ids.loculus.org

@maverbiest maverbiest added the preview Triggers a deployment to argocd label Sep 10, 2026
@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @maverbiest's task in 1m 9s —— View job


Claude Code Review

  • Read PR diff
  • Trace developmentDatabasePersistence through the chart templates
  • Post review

Summary

One line, kubernetes/loculus/values.yaml:2934, flipping developmentDatabasePersistence from false to true. This is the documented way to get a persistent DB on a preview (kubernetes/README.md:135), and the PR is explicitly marked not-to-be-merged, so the change itself is fine for its purpose.

Two things to flag:

1. The branch doesn't contain the thing the title says it tests. The title is "testing new file ids on preview", but the only commit is Make DB persistent and the diff touches nothing related to file IDs. If the goal is to exercise new file-ID behaviour on a preview instance, that code needs to be on this branch too (or the branch rebased onto whatever contains it) — as it stands the preview will run main's code with a persistent DB.

2. Don't merge this. values.yaml is the chart-wide default, so merging would turn persistence on for every deployment that doesn't override it, including values_e2e_and_dev.yaml and all other previews (neither values_preview_server.yaml nor values_e2e_and_dev.yaml sets it). Consider marking the PR as draft to guard against an accidental merge. Note also that values.schema.json:1687 already documents the default as true while values.yaml shipped false — a pre-existing mismatch that this change coincidentally papers over. Worth fixing the schema separately.

Side effects of the flag beyond DB persistence (MinIO object retention, silo Force=true sync, valuesHash restarts, and the fact that there's no PVC behind "persistent") are in the inline comment.

No code-quality, security, performance, test, or documentation concerns apply to a one-line values flip on a throwaway branch.
· branch test-new-file-ids

@claude claude Bot added the deployment Code changes targetting the deployment infrastructure label Sep 10, 2026
@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

This PR may be related to: #6907

Comment thread kubernetes/loculus/values.yaml
@maverbiest
maverbiest marked this pull request as draft September 10, 2026 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deployment Code changes targetting the deployment infrastructure preview Triggers a deployment to argocd

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants