Skip to content

Conversation

@ainsleyclark
Copy link
Contributor

Summary

Fixed the issue where backup.enabled and monitoring fields were unconditionally overridden to true during the webkit update command, preventing users from disabling these features even when explicitly configured.

Changes

  • Struct Definitions: Changed Backup.Enabled and Monitoring fields from bool to *bool to allow distinguishing between "not set" and "explicitly false"
  • Default Application: Updated applyDefaults() methods to conditionally apply defaults only when fields are nil (using ptr.BoolPtr() for consistency)
  • Helper Methods: Added IsBackupEnabled(), IsMonitoringEnabled() accessor methods following existing patterns
  • Field Access: Updated all field access sites to use helper methods instead of direct field access, preventing nil pointer issues
  • Tests: Updated all test cases to use ptr.BoolPtr() for creating pointers, added test cases for disabled scenarios

Testing

  • All Go tests pass
  • Go linter: 0 issues
  • JS linter: No issues
  • Code compiles successfully

## Summary

Fixed the issue where backup.enabled and monitoring fields were unconditionally
overridden to true during the webpack update command, preventing users from
disabling these features even when explicitly configured.

## Changes

- **Struct Definitions**: Changed `Backup.Enabled` and `Monitoring` fields from
  bool to *bool to allow distinguishing between "not set" and "explicitly false"
- **Default Application**: Updated applyDefaults() methods to conditionally apply
  defaults only when fields are nil (using ptr.BoolPtr() for consistency)
- **Helper Methods**: Added IsBackupEnabled(), IsMonitoringEnabled() accessor
  methods following existing patterns (e.g., IsTerraformManaged())
- **Field Access**: Updated all field access sites to use helper methods instead
  of direct field access, preventing nil pointer issues
- **Tests**: Updated all test cases to use ptr.BoolPtr() for creating pointers,
  added test cases for disabled scenarios

## Key Improvements

- Users can now disable backups with `"backup": {"enabled": false}`
- Users can now disable monitoring with `"monitoring": false`
- 100% backward compatible - omitted fields still default to true
- Matches existing codebase pattern (TerraformManaged field)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@claude

This comment has been minimized.

Changed template to call the IsMonitoringEnabled() method on resources instead of directly accessing the Monitoring pointer field. This correctly handles nil pointers and respects user-provided false values.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@claude
Copy link

claude bot commented Nov 30, 2025

Review summary

  • Overall score: 9/10
  • Critical issues: 0
  • Warnings: 0
  • Suggestions: 1
  • Recommendation: ✅ Approve

Excellent implementation that correctly addresses the configuration override bug. The PR demonstrates strong adherence to project patterns with proper use of pointer types, helper methods, and comprehensive test coverage.

Critical issues 🔴

None

Warnings 🟡

None

Suggestions 🟢

Consider consistency in applyDefaults comment style

Location: internal/appdef/resources.go:175-182 and internal/appdef/apps.go:312-315

The comment style differs slightly between the two implementations:

// resources.go
// Apply backup default only if not explicitly set.
if r.Backup.Enabled == nil {
    r.Backup.Enabled = ptr.BoolPtr(true)
}

// Apply monitoring default only if not explicitly set.
if r.Monitoring == nil {
    r.Monitoring = ptr.BoolPtr(true)
}

// apps.go
// Apply monitoring default only if not explicitly set.
if a.Monitoring == nil {
    a.Monitoring = ptr.BoolPtr(true)
}

Consider using consistent phrasing across both files. For example, using "only if not explicitly set" or "when field is nil" consistently throughout. This is a minor style point and doesn't affect functionality.


What this PR does well:

  1. Correct use of pointer types: Changing bool to *bool allows proper tri-state logic (nil/true/false)
  2. Helper method pattern: IsBackupEnabled() and IsMonitoringEnabled() follow existing patterns like IsTerraformManaged()
  3. Comprehensive test coverage: Added tests for all three states (nil, true, false) across both resources and apps
  4. Template compatibility: Template changes correctly use helper methods instead of direct field access
  5. Backwards compatibility: Defaults to true when nil, maintaining existing behaviour for users who haven't explicitly set these fields
  6. Consistent application: All usage sites properly updated across 11 files

The implementation correctly solves the original issue where webkit update was unconditionally overriding user configuration.

@codecov
Copy link

codecov bot commented Nov 30, 2025

Codecov Report

❌ Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 69.43%. Comparing base (7f6b060) to head (1b49550).
⚠️ Report is 419 commits behind head on main.

Files with missing lines Patch % Lines
internal/appdef/monitor.go 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #341      +/-   ##
==========================================
+ Coverage   64.59%   69.43%   +4.83%     
==========================================
  Files         154      184      +30     
  Lines        6064     7263    +1199     
==========================================
+ Hits         3917     5043    +1126     
+ Misses       2064     2023      -41     
- Partials       83      197     +114     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ainsleyclark ainsleyclark merged commit eacb38c into main Nov 30, 2025
6 checks passed
@ainsleyclark ainsleyclark deleted the fix/defaults branch November 30, 2025 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants