Skip to content

feat(config): start using outputFileUrlType website on previews - #7301

Open
anna-parker wants to merge 1 commit into
mainfrom
outputFileUrlType-website
Open

feat(config): start using outputFileUrlType website on previews#7301
anna-parker wants to merge 1 commit into
mainfrom
outputFileUrlType-website

Conversation

@anna-parker

@anna-parker anna-parker commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @anna-parker's task in 2m 13s —— View job


Review

One-line config flip. Left one inline comment on values.yaml:64.

Main point: the change is in kubernetes/loculus/values.yaml, which is the chart-wide default, not preview-scoped — despite the PR title. Preview-only config belongs in values_preview_server.yaml.

Things I checked that look fine:

  • The website route exists and matches the URL the backend builds (ReleasedDataModel.kt:255website/src/pages/seq/[accessionVersion]/[fileCategory]/[fileName].ts), including the logged-out 401 fix from fix(website): Fix outputFileUrlType: website file URLs resulting in 401 for logged out users #7289.
  • No bandwidth concern: /files/get always returns a 307 to a presigned S3 URL (FilesController.kt:101), and the website route forwards the Location rather than streaming bytes through Astro.
  • The website config also receives fileSharing (_common-metadata.tpl:243), but fileSharingConfig (website/src/types/config.ts:240) is a non-strict zod object, so the extra outputFileUrlType key is stripped — no validation failure.
  • No test hardcodes the URL shape (integration-tests/tests/specs/features/file-sharing.spec.ts), so e2e should pass, and since values_e2e_and_dev.yaml doesn't override this, CI now actually exercises the website path.

Minor: outputFileUrlType is a required field in values.schema.json but isn't mentioned anywhere in docs/. If the shipped default is changing, a line in the admin docs explaining the three modes would help chart consumers.

Todos: - [x] Gather context - [x] Analyze semantics/downstream effects - [x] Check preview vs. chart-wide scope - [x] Post review
· branch outputFileUrlType-website

@claude claude Bot added the deployment Code changes targetting the deployment infrastructure label Sep 10, 2026
Comment thread kubernetes/loculus/values.yaml
@tombch tombch added the preview Triggers a deployment to argocd label Sep 10, 2026
@tombch

tombch commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

The integration tests seem to be showing a CORS issue

(I am having a look with claude)

@tombch

tombch commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

I did just test making a dummy submission with files though and it seems to work!

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