Skip to content

[SHOT-187](feat: SH) - Add support for migrationTimeoutSeconds - #578

Open
mimartin12 wants to merge 3 commits into
mainfrom
SHOT-187-migration-timeout
Open

[SHOT-187](feat: SH) - Add support for migrationTimeoutSeconds#578
mimartin12 wants to merge 3 commits into
mainfrom
SHOT-187-migration-timeout

Conversation

@mimartin12

@mimartin12 mimartin12 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

🎟️ Tracking

https://bitwarden.atlassian.net/browse/SHOT-187
https://bitwarden.atlassian.net/browse/PM-37891

📔 Objective

Adds an input for migrationTimeoutSeconds, which is advanced configuration for users who may have large databases and require more time to run database migrations.

See bitwarden/server#8143

@mimartin12 mimartin12 added the ai-review Request a Claude code review label Aug 5, 2026
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the new jobs.db.migrationTimeoutSeconds option, which is plumbed through as a -t argument on both the pre-install and post-install MSSQL migrator jobs and as a globalSettings__sqlServer__migrationExecutionTimeoutSeconds entry in the pre-install hook ConfigMap. The option is opt-in and defaults to empty, so rendered output is unchanged for existing installs; the post-install if/else if branching preserves the prior transition-args behavior for both fresh installs and upgrades. The regenerated values.schema.json matches the new values.yaml entry, and new unit tests cover the ConfigMap key and all three post-install arg permutations.

Code Review Details
  • ♻️ : New args logic in the pre-install migrator job has no helm-unittest coverage, unlike the equivalent post-install change
    • charts/self-host/templates/pre-install-db-migrator-job.yaml:97-99
  • 🎨 : Document the unlimited-timeout value as quoted "0"; an unquoted 0 fails values.schema.json type validation
    • charts/self-host/values.yaml:891-894

Both findings already have open inline threads from the previous review pass; no new inline comments were added.

PR Metadata Assessment

  • ❓ : The version:patch label is applied, but CONTRIBUTING.md maps "new configuration options" to version:minor.

@mimartin12 mimartin12 added the version:patch Backwards-compatible fixes and maintenance improvements. label Aug 5, 2026
Comment thread charts/self-host/values.yaml
Comment thread charts/self-host/templates/pre-install-db-migrator-job.yaml
@mimartin12 mimartin12 added version:minor New functionality added in a backwards-compatible manner. and removed version:patch Backwards-compatible fixes and maintenance improvements. labels Aug 6, 2026
mimartin12 and others added 2 commits August 6, 2026 14:32
…ator tests

Clarify that the unlimited-timeout value must be quoted, since the schema
types migrationTimeoutSeconds as a string and an unquoted 0 fails validation.

Add helm-unittest coverage for the pre-install migrator job args, including
the "0" unlimited case so a future schema widening to integers fails a test
rather than silently dropping the flag.
@mimartin12
mimartin12 marked this pull request as ready for review August 6, 2026 21:02
@mimartin12
mimartin12 requested a review from a team as a code owner August 6, 2026 21:02
@mimartin12
mimartin12 requested a review from keithhubner August 6, 2026 21:02
@mimartin12 mimartin12 added the hold Hold this PR or item until later; DO NOT MERGE label Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review hold Hold this PR or item until later; DO NOT MERGE version:minor New functionality added in a backwards-compatible manner.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants