feat(config): seed server address from environment#5785
Draft
zhangzhichaolove wants to merge 2 commits into
Draft
feat(config): seed server address from environment#5785zhangzhichaolove wants to merge 2 commits into
zhangzhichaolove wants to merge 2 commits into
Conversation
Add SERVER_ADDRESS as a first-run seed for the ServerAddress option. When the database does not yet contain a ServerAddress row, startup persists the environment value through the existing option update path so runtime state and the options table stay in sync. Existing database/admin settings continue to take precedence and are never overwritten by the environment variable. Document the new variable in README files and docker-compose examples, and cover both seed and no-overwrite behavior with model tests. Refs QuantumNous#5549
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Walkthrough
ChangesSERVER_ADDRESS First-Run Seed
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
model/option_test.go (1)
21-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClone
OptionMapbefore saving/restoring it.
previousOptionMap := common.OptionMaponly copies the map header. IfInitOptionMapor a future test mutates the existing map in place, cleanup restores the already-mutated contents and leaks state into later tests.Suggested fix
common.OptionMapRWMutex.RLock() - previousOptionMap := common.OptionMap + previousOptionMap := make(map[string]string, len(common.OptionMap)) + for k, v := range common.OptionMap { + previousOptionMap[k] = v + } common.OptionMapRWMutex.RUnlock()Also applies to: 39-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/option_test.go` around lines 21 - 23, The test setup around common.OptionMap is only keeping a shallow reference, so later mutations can leak between tests; update the save/restore logic in the OptionMap test helpers to clone the map contents before storing previousOptionMap and when restoring it. Use the existing common.OptionMapRWMutex-protected section in option_test.go and ensure both the initial snapshot and the cleanup assignment operate on independent copies rather than the shared map header.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@model/option_test.go`:
- Line 52: The test queries in option_test.go are using dialect-specific
backtick quoting, which breaks PostgreSQL compatibility; update the affected
assertions in the option test flow (including the DB.First calls around the
ServerAddress and related key lookups) to let GORM build the predicate instead
of hardcoding SQL quoting. Use GORM’s structured condition APIs with the
existing option model/query path so the same test works across SQLite, MySQL,
and PostgreSQL.
In `@model/option.go`:
- Around line 190-209: loadOptionsFromDatabase currently ignores the error from
AllOption(), which can make a database read failure look like ServerAddress is
missing and trigger unwanted seeding. Update loadOptionsFromDatabase to handle
the AllOption() error explicitly and return or abort before checking
serverAddressExists or calling UpdateOption, so seeding only happens after a
successful options load.
---
Nitpick comments:
In `@model/option_test.go`:
- Around line 21-23: The test setup around common.OptionMap is only keeping a
shallow reference, so later mutations can leak between tests; update the
save/restore logic in the OptionMap test helpers to clone the map contents
before storing previousOptionMap and when restoring it. Use the existing
common.OptionMapRWMutex-protected section in option_test.go and ensure both the
initial snapshot and the cleanup assignment operate on independent copies rather
than the shared map header.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ed99172a-e805-4aae-9072-249f1c02d144
📒 Files selected for processing (11)
README.en.mdREADME.fr.mdREADME.ja.mdREADME.mdREADME.zh_CN.mdREADME.zh_TW.mddocker-compose.dev.ymldocker-compose.ymldocs/installation/BT.mdmodel/option.gomodel/option_test.go
Handle option load errors before seeding SERVER_ADDRESS, avoid dialect-specific test predicates, and clone OptionMap snapshots in tests. Refs QuantumNous#5549
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
📝 变更描述 / Description
为
ServerAddress增加SERVER_ADDRESS环境变量配置。启动加载系统选项时，如果数据库中还没有
ServerAddress配置时，并且环境变量SERVER_ADDRESS不为空，后端会通过现有UpdateOption路径写入该值。这样它会和管理员在 UI 中保存配置一样，同步到运行时的system_setting.ServerAddress。如果数据库中已经存在
ServerAddress，环境变量不会覆盖已有值，保持优先级为：数据库/管理员配置 > 环境变量设置值 > 代码默认值。同时补充了
SERVER_ADDRESS的 README 环境变量说明、宝塔安装文档说明，以及docker-compose.yml/docker-compose.dev.yml示例注释。🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
验证覆盖：
ServerAddress时，SERVER_ADDRESS会写入 options 表并同步运行时配置。ServerAddress时，SERVER_ADDRESS不会覆盖管理员配置。Summary by CodeRabbit
New Features
SERVER_ADDRESSenvironment variable to seed the initial server address on first startup when no existing setting is present.SERVER_ADDRESSusage.Bug Fixes
SERVER_ADDRESSdoes not overwrite an existing, already-configured server address.Tests