opt-in to disable automatic DB migration #9449
opt-in to disable automatic DB migration #9449contributor wants to merge 4 commits intoTriliumNext:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to disable automatic database migrations using the TRILIUM_MANUAL_DB_MIGRATION environment variable. Feedback suggests localizing the error message and integrating this setting into the central configuration system for better consistency and flexibility.
| if (process.env.TRILIUM_MANUAL_DB_MIGRATION === "true") { | ||
| await crash(`Automatic DB migrations are disabled via env var TRILIUM_MANUAL_DB_MIGRATION = ${process.env.TRILIUM_MANUAL_DB_MIGRATION}`); | ||
| } |
There was a problem hiding this comment.
The error message should be localized using the t() function to maintain consistency with the rest of the application. Furthermore, this configuration option should ideally be integrated into the central configuration system in apps/server/src/services/config.ts. This would allow it to be documented in config-sample.ini, be configurable via config.ini, and benefit from the project's standard environment variable resolution (which handles case-insensitivity and boolean variations like 1/0).
| if (process.env.TRILIUM_MANUAL_DB_MIGRATION === "true") { | |
| await crash(`Automatic DB migrations are disabled via env var TRILIUM_MANUAL_DB_MIGRATION = ${process.env.TRILIUM_MANUAL_DB_MIGRATION}`); | |
| } | |
| if (process.env.TRILIUM_MANUAL_DB_MIGRATION?.toLowerCase() === "true") { | |
| await crash(t("migration.manual_db_migration_disabled", { value: process.env.TRILIUM_MANUAL_DB_MIGRATION })); | |
| } |
There was a problem hiding this comment.
@gemini-code-assist the use case is very niche, mainly for developers or users who runs multiple app versions at the same time. There is not benefit to add more code for translations or config.ts
There was a problem hiding this comment.
I understand that this is a niche use case. However, even for niche features, it is best practice to follow the established patterns of the codebase to ensure maintainability and consistency. Using the central configuration system ensures that the setting is discoverable and consistent with how other environment variables are handled across the application, rather than relying on direct process.env access in a service file.
There was a problem hiding this comment.
@contributor , it's fine to add a translation for it. Please also mention it somewhere in the documentation. Even if it's niche.
cbf279f to
2474933
Compare
| if (process.env.TRILIUM_MANUAL_DB_MIGRATION === "true") { | ||
| await crash(`Automatic DB migrations are disabled via env var TRILIUM_MANUAL_DB_MIGRATION = ${process.env.TRILIUM_MANUAL_DB_MIGRATION}`); | ||
| } |
There was a problem hiding this comment.
@contributor , it's fine to add a translation for it. Please also mention it somewhere in the documentation. Even if it's niche.
@eliandoran added translation and documentation The code still compares the env var strictly to "true". This contradicts the docs that allows true/1. Looks like I will have to add this to |
This intended mostly for developers.
If you opt-in to set TRILIUM_MANUAL_DB_MIGRATION, you won't accidentally upgrade DB when switching between branches