-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Add Uptime Kuma monitoring integration #269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Integrates Uptime Kuma monitoring for apps, resources, and backup jobs. Monitoring is enabled by default (opt-out) with simple user configuration and sophisticated internal defaults. Changes: - Add MonitoringConfig to schema.json for apps and resources - Create appdef.Monitor struct with smart defaults for HTTP, Postgres, and Push monitors - Update App and Resource structs with monitoring field (defaults to enabled) - Add Uptime Kuma Terraform provider configuration - Create monitoring module at platform/terraform/modules/monitoring - Generate monitors from apps (all domains) and resources (Postgres only) - Add heartbeat monitor support for backup jobs Monitoring features: - HTTP monitors: All app domains (primary + aliases) - Postgres monitors: Database connection health checks - Push monitors: Heartbeat tracking for CI backup jobs - Auto-calculated intervals with sensible defaults Still TODO: - Backup workflow heartbeat integration - Tests and documentation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Comprehensive documentation covering: - Configuration and setup - Monitor types (HTTP, Postgres, Push) - Implementation architecture - Troubleshooting guide - Future enhancements Phase 5 (backup heartbeat integration) documented as TODO. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
PR Review: Uptime Kuma Monitoring IntegrationSummaryThis PR adds comprehensive Uptime Kuma monitoring integration for WebKit-managed applications and resources. The implementation is well-structured and follows good design patterns. However, there are several areas that need attention before merging. Code QualityStrengths
IssuesCritical: Missing Test CoverageLocation: internal/appdef/monitor.go The new monitor.go file (216 lines) has ZERO test coverage. According to AGENTS.md guidelines, all exported functions should have comprehensive tests. Missing tests for:
Required Action: Add monitor_test.go with test tables covering:
High: Hardcoded Cron ScheduleLocation: internal/infra/tf_vars.go:281 cronSchedule := "0 2 * * *" // Daily at 2am (from backup template).This hardcoded value is passed to calculateHeartbeatInterval() but is ignored anyway (see next issue). The cron schedule should come from the backup configuration, not be hardcoded here. Recommendation: Either:
Medium: Incomplete ImplementationLocation: internal/appdef/monitor.go:190-206 The calculateHeartbeatInterval() function:
This means ALL backup monitors will have the same interval regardless of their actual schedule, which could lead to false alerts for non-daily backups. Recommendation:
Medium: Empty Default for Notification IDsLocation: internal/infra/tf_vars.go:316-320 The getNotificationIDs() function always returns an empty array, meaning monitors won't send notifications until manually configured in Uptime Kuma UI. This reduces the value of automated monitor creation. Recommendation:
Low: Inconsistent Error HandlingLocation: internal/appdef/monitor.go:149-162 GenerateHeartbeatMonitor() returns an empty Monitor{} when backup is disabled, but callers check heartbeat.Enabled. This works but is inconsistent with GenerateMonitors() which returns nil. Recommendation: Return nil for consistency, or add a comment explaining why empty Monitor is preferred. Best PracticesFollow Go Guidelines (AGENTS.md)
Issues
Performance Considerations
Security ConcernsCritical: Database URL ExposureLocation: internal/appdef/monitor.go:136 DatabaseURL: r.terraformOutputReference(enviro, "connection_url"),This creates a Terraform interpolation string that will contain the full database connection URL (including credentials) in:
Mitigation: The outputs.tf correctly marks postgres_monitors as sensitive=true (line 20), but ensure:
Medium: TLS Validation Disabled OptionLocation: internal/appdef/monitor.go:91, 142 IgnoreTLS is always set to false, which is correct. However, the field exists and could be accidentally enabled. Consider:
Schema ValidationLocation: schema.json The MonitoringConfig type is correctly defined but very minimal. Consider adding:
DocumentationStrengths
Suggestions
Breaking ChangesThe PR claims "None" but there's a subtle issue: Location: internal/appdef/apps.go:273 and resources.go:166 Monitoring is enabled by default in applyDefaults(). For existing projects:
Recommendation:
Terraform Provider ConcernsLocation: platform/terraform/base/main.tf:75-79 The uptimekuma provider configuration uses basic auth. Verify:
Test Coverage RequirementsAccording to AGENTS.md, you MUST add:
Example test structure needed: func TestApp_GenerateMonitors(t *testing.T) {
t.Parallel()
tt := map[string]struct {
app *App
want []Monitor
}{
"Monitoring disabled": {
app: &App{Monitoring: MonitoringConfig{Enabled: false}},
want: nil,
},
"No domains": {
app: &App{Monitoring: MonitoringConfig{Enabled: true}, Domains: []Domain{}},
want: []Monitor{},
},
// ... more cases
}
for name, test := range tt {
t.Run(name, func(t *testing.T) {
t.Parallel()
got := test.app.GenerateMonitors()
assert.Equal(t, test.want, got)
})
}
}RecommendationsBefore Merge (Required)
Nice to Have (Post-Merge)
VerdictThis is solid foundational work with good architecture, but should not be merged without test coverage. The missing tests are a critical gap that violates the project's testing guidelines. Once tests are added and the hardcoded cron schedule is addressed, this will be ready to merge. Great work on the comprehensive implementation. The architecture is sound and well-documented. Focus on adding those tests and this will be an excellent addition to WebKit. |
Improvements based on code review: - Rename MonitoringConfig to Monitoring (simpler, more consistent) - Add reference to Uptime Kuma API docs in Monitor struct - Remove unnecessary IsMonitoringEnabled helper methods (direct field access) - Move terraformOutputReference from appdef to infra layer (proper separation) - Add fmt import to tf_vars.go Changes adhere to proper layer separation: - appdef: Pure domain definitions - infra: Terraform-specific logic and references Reference: https://github.com/louislam/uptime-kuma/wiki/API-Documentation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Tests added: - internal/appdef/monitor_test.go (14 tests, 47 assertions) - MonitorType String conversion - App.GenerateMonitors() with various scenarios - App.healthCheckPath() edge cases - Resource.GenerateMonitors() for different resource types - Resource.GenerateHeartbeatMonitor() for backups - Helper functions (calculateHeartbeatInterval, sanitiseMonitorName) - internal/infra/tf_vars_test.go (4 new test functions) - terraformOutputReference() Terraform interpolation - generateMonitors() with apps, resources, and mixed scenarios - tfMonitorFromAppdef() struct transformation - getNotificationIDs() placeholder behaviour Test coverage: - ✅ Monitoring enabled/disabled scenarios - ✅ Multiple domains (primary + aliases) - ✅ Unmanaged domains skipped - ✅ Health check path resolution (custom vs default) - ✅ Postgres vs S3 resource filtering - ✅ Backup heartbeat monitor generation - ✅ Mixed app/resource monitor generation - ✅ Terraform output reference formatting All tests follow WebKit conventions: - Parallel execution where safe - Test tables for simple cases - t.Run subtests for complex scenarios - Clear naming and assertions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Remove three unused fields from the Monitor struct that provided no functional value: - HealthCheckPath: Redundant as the path is already embedded in the URL - ConnectionType: Redundant as the type is already in the DatabaseURL scheme - PushURL: Never used anywhere in the codebase The separation between URL (for HTTP monitors) and DatabaseURL (for database monitors) is intentionally preserved as it accurately mirrors the Uptime Kuma API's polymorphic design where different monitor types use different connection fields (url vs databaseConnectionString). This reduces maintenance burden and testing surface area while keeping the struct aligned with the actual API requirements. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Overview
Integrates Uptime Kuma monitoring for WebKit-managed applications and resources. This PR implements automatic uptime monitoring for apps (HTTP), Postgres databases, and backup jobs with heartbeat tracking.
Motivation
As discussed in #267, WebKit-enabled repos now need automatic uptime alerts integrated with the existing Uptime Kuma instance at https://uptime.ainsley.dev/. This provides:
Implementation
Phase 1: Schema & Appdef Changes
MonitoringConfigtype toschema.jsonwith simpleenabled: truefieldinternal/appdef/monitor.gowith sophisticated internalMonitorstructAppandResourcestructs with monitoring field (defaults to enabled)Phase 2: Terraform Provider Integration
ehealth-co-id/uptimekumaTerraform provideruptime_kuma_url,uptime_kuma_username,uptime_kuma_passwordPhase 3: Monitoring Module
platform/terraform/modules/monitoring/modulePhase 4: Variable Generation
internal/infra/tf_vars.goto generate monitors from appdefappdef.MonitortotfMonitorfor Terraform consumptionPhase 6: Documentation
docs/monitoring.mdConfiguration
User Configuration (Minimal)
Monitoring is enabled by default (opt-out). To disable:
{ "apps": [ { "name": "web", "monitoring": { "enabled": false } } ], "resources": [ { "name": "db", "monitoring": { "enabled": false } } ] }Uptime Kuma Credentials
Add to
.env.production.enc:Monitor Types
HTTP Monitors
health_check_path(default:/)Postgres Monitors
Push Monitors (Infrastructure Ready)
What's Monitored
For this example
app.json:{ "apps": [ { "name": "web", "domains": [ { "name": "example.com", "type": "primary" }, { "name": "www.example.com", "type": "alias" } ] } ], "resources": [ { "name": "db", "type": "postgres", "provider": "digitalocean" } ] }Monitors created:
project-web-example-com(HTTP)project-web-www-example-com(HTTP)project-db-production(Postgres)project-backup-db(Push - heartbeat)Testing
TODO (Future Work)
Phase 5: Backup Heartbeat Integration
Future Enhancements
Breaking Changes
None. This is purely additive:
Files Changed
Created:
internal/appdef/monitor.go- Monitor generation logicplatform/terraform/modules/monitoring/- Monitoring Terraform moduledocs/monitoring.md- DocumentationModified:
schema.json- Added MonitoringConfig typeinternal/appdef/apps.go- Added Monitoring fieldinternal/appdef/resources.go- Added Monitoring fieldinternal/infra/tf_vars.go- Monitor generationplatform/terraform/base/main.tf- Provider & module integrationplatform/terraform/base/variables.tf- Monitor variablesRelated Issues
Addresses requirements from #267 (monitoring/alerts discussion)
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com