Improve env config handling and runtime defaults

[tanji]

Summary

• standardize env prefixes and precedence, plus env-based cluster discovery and secret propagation
• improve runtime defaults with non-root working dir and key path fallback, and guard Cloud18 config read
• refine mail init behavior and add implementation docs/tests

Testing

• go test -vet=off ./server

[claude]

PR Review: Environment Config Handling and Runtime Defaults

Strengths

✅ Environment Variable Standardization: Clear, consistent prefix scheme (REPLICATION_MANAGER_DEFAULT_*, REPLICATION_MANAGER_<CLUSTER>_*) improves usability.

✅ Cluster Discovery from Env: Dynamic cluster detection from environment variables eliminates the need for explicit cluster configuration, useful for containerized deployments.

✅ Non-root Friendly Defaults: Fallback to $HOME/.local/replication-manager/data for working directory is practical for non-root deployments.

✅ Security Improvements:

• Key path fallback mechanism avoids hardcoded paths (/home/repman)
• SQL injection prevention in dbhelper (parametrized queries, identifier validation)
• Nil guards added to ReadCloud18Config

✅ Configuration Precedence: Well-documented precedence: config file < env vars < CLI flags, properly enforced.

✅ Test Coverage: 118+ new test cases covering env overrides, precedence, and cluster discovery.

✅ Documentation: Implementation docs clearly explain changes.

Issues & Concerns

⚠️ Duplicate Code in Config Processing (HIGH)

• Lines 1545-1548 in server.go: DeprecatedKeys assignment is duplicated:

if clRead != nil {
    repman.DeprecatedKeys[clusterName] = repman.GetUsedAliasKeys(clRead, true)
}
clOrch := conf.ProvOrchestrator
if clRead != nil {
    repman.DeprecatedKeys[clusterName] = repman.GetUsedAliasKeys(clRead, true) // DUPLICATE
}

Remove the first occurrence.

⚠️ Cluster Discovery Efficiency (MEDIUM)

• discoverClustersFromEnv() rebuilds suffix list on every call and iterates all environment variables O(n). Consider caching or using env var prefix filtering if performance matters with many env vars.

⚠️ Working Directory Creation Missing Error Handling (MEDIUM)

• When conf.WorkingDir is set based on home directory fallback, the code doesn't verify the directory is actually created. Should add:

if err := os.MkdirAll(conf.WorkingDir, 0755); err != nil {
    // Handle error
}

⚠️ Mailer Guard Logic (MINOR)

• isInitMailEnabled() and mailerConfigMissing() methods referenced but not shown in diff. Verify these properly check all required SMTP fields (host, port, user, password).

⚠️ Docker Changes (MINOR)

• Removed cluster config file copies and mariadb-server package install in Docker builds. This is a breaking change—ensure it's intentional and documented. The new docker-compose.yml example is good but doesn't mirror the removed configs.

⚠️ Type Conversion Risk (MINOR)

• dbhelper changes use ValidateIdentifier() and QuoteMySQLIdentifier() for SQL injection prevention. These are good, but ensure they handle all edge cases (backticks in identifiers, reserved keywords).

Test Coverage Gaps

⚠️ Missing Tests:

• Non-root working directory fallback creation
• Nil pointer handling in ReadCloud18Config (only guards added, no test shown)
• Mailer initialization guard behavior with missing config
• Docker compose multi-node cluster topology validation

Recommendations

1. Fix duplicate code in cluster config processing immediately
2. Add directory creation validation for non-root working dir fallback
3. Document Docker breaking changes or restore removed configs
4. Add tests for working directory creation and mailer guard conditions
5. Consider caching env discovery results if cluster count is large

Overall Assessment

Quality: Good | Risk: Low-Medium

The PR improves operational flexibility and security significantly. Main concerns are code duplication and missing validation in the working directory fallback logic. These are straightforward fixes that don't require architectural changes.