Skip to content

fix: wrong defaulting of setting enableDatabaseSync to false - #275

Merged
GrigoryPervakov merged 1 commit into
mainfrom
fix-enable-database-sync-optional
Jul 21, 2026
Merged

fix: wrong defaulting of setting enableDatabaseSync to false#275
GrigoryPervakov merged 1 commit into
mainfrom
fix-enable-database-sync-optional

Conversation

@GrigoryPervakov

Copy link
Copy Markdown
Member

Why

omitempty wipes the false value from JSON, and the API server sets the value to default=true

What

Make it a pointer to distinguish a false and "not specified" case.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a bug in the ClickHouseCluster API where spec.settings.enableDatabaseSync: false could be dropped during JSON serialization due to omitempty, causing the API server to apply the CRD default (true) instead. It does so by making the field a pointer so the operator can distinguish “not specified” (nil) from an explicit false.

Changes:

  • Change ClickHouseSettings.EnableDatabaseSync from bool to *bool to preserve explicit false values.
  • Update ClickHouse reconciliation logic to treat nil as enabled (default), and skip only when explicitly false.
  • Regenerate deepcopy logic for the new pointer field.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.

File Description
internal/controller/clickhouse/sync.go Updates database-sync gating logic to handle pointer semantics correctly (nil vs explicit false).
api/v1alpha1/zz_generated.deepcopy.go Regenerated deepcopy to correctly copy the new *bool field.
api/v1alpha1/clickhousecluster_types.go Changes the API type of enableDatabaseSync to *bool while keeping the CRD default behavior.
Files not reviewed (1)
  • api/v1alpha1/zz_generated.deepcopy.go: Generated file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread api/v1alpha1/clickhousecluster_types.go
Comment thread internal/controller/clickhouse/sync.go
@GrigoryPervakov
GrigoryPervakov enabled auto-merge (squash) July 21, 2026 17:03
@GrigoryPervakov
GrigoryPervakov merged commit b36e1c7 into main Jul 21, 2026
39 of 43 checks passed
@GrigoryPervakov
GrigoryPervakov deleted the fix-enable-database-sync-optional branch July 21, 2026 17:12
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