fix: Default Values

[ainsleyclark]

Changed Backup and Monitoring fields to pointer types to properly distinguish between user-specified false values and unset defaults. This fixes the issue where setting backup.enabled to false would be overwritten to true on webkit update.

• Changed Resource.Backup from ResourceBackupConfig to *ResourceBackupConfig
• Changed Resource.Monitoring from bool to *bool
• Changed App.Monitoring from bool to *bool
• Updated applyDefaults() to only set defaults when fields are nil
• Updated all usages and tests to use ptr.BoolPtr() helper
• Updated README template to handle nil backup config

[claude]

Review summary

• Overall score: 8/10
• Critical issues: 0
• Warnings: 2
• Suggestions: 1
• Recommendation: ⚠️ Approve with comments

This PR properly fixes the default value handling issue by converting boolean and backup config fields to pointers. The implementation is clean and follows Go best practices. However, there's a critical bug in the template logic that needs addressing.

Critical issues 🔴

None

Warnings 🟡

1. Template logic error in backup.yaml.tmpl (lines 87, 155, 225)

Location: internal/templates/.github/workflows/backup.yaml.tmpl:87,155,225

The template condition is inverted:

{{- if and $.MonitoringEnabled (or (not .Monitoring) (deref .Monitoring)) }}

This logic reads as: "If monitoring is enabled globally AND (monitoring field is nil OR monitoring is true)". However, this will always evaluate to true when monitoring is enabled globally, even when a user explicitly sets monitoring: false.

Problem: When .Monitoring is nil, (not .Monitoring) evaluates to true, so the condition passes. When .Monitoring is a pointer to false, (deref .Monitoring) returns false, but (not .Monitoring) returns false (since not on a non-nil pointer is false), making the entire or still evaluate incorrectly.

Expected logic: "Include monitoring step if global monitoring is enabled AND resource monitoring is not explicitly disabled"

Correct condition:

{{- if and $.MonitoringEnabled (or (not .Monitoring) (and .Monitoring (deref .Monitoring))) }}

Or more simply using a helper pattern:

{{- if and $.MonitoringEnabled (ne (deref .Monitoring) false) }}

But the best approach is to check if it's nil OR true:

{{- if and $.MonitoringEnabled (or (eq .Monitoring nil) (deref .Monitoring)) }}

See internal/appdef/monitor.go:303 for the correct Go pattern you're using elsewhere:

if resource.Monitoring \!= nil && \!*resource.Monitoring {
    continue
}

2. Inconsistent nil-check patterns

Location: internal/appdef/apps.go:346-349, monitor.go:260-263, etc.

The code uses two different patterns for the same logic:

Pattern A (apps.go:346):

if a.Monitoring \!= nil && \!*a.Monitoring {
    return nil
}

Pattern B (monitor.go:260):

if app.Monitoring \!= nil && \!*app.Monitoring {
    continue
}

While both work correctly, the comment "Only skip if monitoring is explicitly disabled" appears above both. Consider extracting this into a helper method for consistency:

func (a *App) IsMonitoringEnabled() bool {
    return a.Monitoring == nil || *a.Monitoring
}

This would make the code more readable: if \!app.IsMonitoringEnabled() { continue }

Suggestions 🟢

1. Consider adding tests for nil vs explicit false in templates

The template logic is complex enough that integration tests verifying the generated workflow YAML would be valuable. Consider adding a test case in internal/cmd/cicd/backup_test.go that checks the generated YAML when monitoring is explicitly set to false.

---

Testing note: All Go tests updated correctly with pointer conversions. The changes maintain backward compatibility since the default behaviour (nil → true) is preserved.