Skip to content

[deploy] Skip database index script when image lacks it - #4375

Merged
imnasnainaec merged 4 commits into
masterfrom
net-installer-version-resilience
Sep 25, 2026
Merged

imnasnainaec merged 4 commits into
masterfrom
net-installer-version-resilience

Conversation

@imnasnainaec

@imnasnainaec imnasnainaec commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

The net installer is built from master, so it bundles master's Helm chart, but it deploys whatever image version is requested. Since #4328, the database postStart hook runs /opt/thecombine/01-indexes.js, which the v3.0.0 combine_database image does not contain. Installing v3.0.0 with the current net installer therefore fails the hook, and the kubelet kills the database container on every start (CrashLoopBackOff, with mongod exiting cleanly after SIGTERM).

This change runs the index script only when the image includes it, and logs a skip otherwise. v3.0.0's backend does not use EditsCollection, so skipping the index is safe there; v3.0.1+ images will include the script and behave as before.

Other chart changes since v3.0.0 were checked for compatibility with v3.0.0 images (frontend readiness probe on port 80, cert-proxy memory values) and need no change.

🤖 Generated with Claude Code


This change is Reviewable


Devin review: https://app.devin.ai/review/sillsdev/TheCombine/pull/4375


Summary by CodeRabbit

Summary

  • Bug Fixes
    • Database startup now continues when the optional indexing script is unavailable and indicates that indexing was skipped. When the script is available, startup still fails if it encounters an error.
  • Installer
    • Installation now rejects Combine versions earlier than 3.0.0. Installer guidance and examples have been updated to reflect the minimum supported version.

The net installer bundles the chart from master but deploys whatever
image version is requested. The v3.0.0 database image does not include
01-indexes.js, so the postStart hook failed and the kubelet killed the
database container on every start.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 435f7948-846b-4829-bf9e-5e0fe1e93d83

📥 Commits

Reviewing files that changed from the base of the PR and between 6b4bd93 and b9c1f2b.

📒 Files selected for processing (2)
  • deploy/helm/thecombine/charts/database/templates/database.yaml
  • deploy/scripts/install-combine.sh
🚧 Files skipped from review as they are similar to previous changes (2)
  • deploy/scripts/install-combine.sh
  • deploy/helm/thecombine/charts/database/templates/database.yaml

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The installer rejects versions below major version 3 and documents v3.0.0 as the minimum. The database post-start hook runs the index script when it exists and logs when it is absent.

Changes

Version compatibility

Layer / File(s) Summary
Minimum version and installation examples
deploy/scripts/install-combine.sh, installer/README.md
The installer rejects versions with a major version below 3. The README documents v3.0.0 as the minimum, updates the install and update examples, and adds an uninstall example that does not specify a version.
Guarded database index execution
deploy/helm/thecombine/charts/database/templates/database.yaml
The hook runs the index script when it exists and exits with the script’s status if execution fails. If the script is absent, the hook logs that indexes are skipped.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to b9c1f

The installer examples match accepted inputs, and the database hook skips the index script when it is absent from older images. No actionable merge-blocking issue remains.

Architecture Summary

Architecture risk: 🔵 Low · up to 6b4bd

The change affects 2 systems.

Changed systems: deploy, installer

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — deploy (service) was modified; 2 changed files map to changed impact.
  • observed — installer (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in deploy/helm/thecombine/charts/database/templates/database.yaml: The post-start hook now checks for /opt/thecombine/01-indexes.js before running it. When present, it runs the script and exits on failure; when absent, it logs that indexes are skipped instead of failing because the script is missing.
  • observed — Modified behavior in deploy/scripts/install-combine.sh: check-opt-value now rejects validly formatted version strings whose major version is below 3; versions 3 and above continue through validation.
  • observed — Modified behavior in installer/README.md: The documented version format example changes from v1.20.0 to v3.0.1? No—the minimum requirement is stated as v3.0.0 or later, and the example version changes from v1.20.0 to v3.0.1 only if shown; the final row documents vn.n.n and v3.0.0 as the minimum.
  • observed — Modified behavior in installer/README.md: The install example changes from v2.2.0 to v3.0.0, and the update example changes from v3.0.0 to v3.0.1. The restart example remains unchanged; a new uninstall example says no version is needed.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: the database index script is skipped when the image does not contain it.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. (1 skipped: 1 …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@imnasnainaec imnasnainaec self-assigned this Sep 25, 2026
@imnasnainaec imnasnainaec added the 🟥High High-priority PR: please review this asap! label Sep 25, 2026
@imnasnainaec
imnasnainaec marked this pull request as ready for review September 25, 2026 16:41
@codecov

codecov Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.26%. Comparing base (8b8c32e) to head (b9c1f2b).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4375   +/-   ##
=======================================
  Coverage   76.26%   76.26%           
=======================================
  Files         305      305           
  Lines       11452    11452           
  Branches     1409     1409           
=======================================
  Hits         8734     8734           
  Misses       2324     2324           
  Partials      394      394           
Flag Coverage Δ
backend 87.55% <ø> (ø)
frontend 67.09% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

coderabbitai[bot]

This comment was marked as resolved.

The bundled charts assume a replica-set database, which the images
before v3.0.0 do not support.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added documentation Improvements or additions to documentation bash labels Sep 25, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@imnasnainaec imnasnainaec left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@imnasnainaec reviewed all commit messages.
Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on jasonleenaylor).

coderabbitai[bot]

This comment was marked as resolved.

@jasonleenaylor jasonleenaylor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. One question inline about the floor going forward; not blocking.

This review was assisted by Claude Fable 5.1.

Comment thread deploy/scripts/install-combine.sh Outdated
The net installer bundles master's charts but can deploy images as old
as v3.0.0, so chart steps that need newer image content must be guarded
rather than the minimum version raised.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@imnasnainaec
imnasnainaec merged commit 3f735c1 into master Sep 25, 2026
21 of 23 checks passed
@imnasnainaec
imnasnainaec deleted the net-installer-version-resilience branch September 25, 2026 21:28
@imnasnainaec imnasnainaec added the bug Something isn't working label Sep 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bash bug Something isn't working deployment documentation Improvements or additions to documentation 🟥High High-priority PR: please review this asap!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants