Skip to content

fix: consistent status message for unchanged resources #1402

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: consistent status message for unchanged resources #1402

wants to merge 1 commit into from

Conversation

@kamiiiel
Copy link
Contributor

@kamiiiel kamiiiel commented Oct 2, 2025

@kamiiiel kamiiiel requested review from a team as code owners October 2, 2025 17:05
@kamiiiel kamiiiel force-pushed the gko-unchanged branch 6 times, most recently from 4914112 to ed1ca9f Compare October 3, 2025 09:48
@kamiiiel kamiiiel force-pushed the gko-unchanged branch 4 times, most recently from ceee328 to 6faac35 Compare October 15, 2025 14:03
gemini-code-assist[bot]
gemini-code-assist bot reviewed Oct 16, 2025
View reviewed changes
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a significant and valuable refactoring by changing numerous struct fields from value types to pointer types. This change allows for better handling of default values and distinguishing between unset fields and those explicitly set to their zero value, which is crucial for consistent behavior, especially in status messages for unchanged resources. The introduction of the adapter package to centralize default value population is a great step towards better maintainability. My review focuses on ensuring the consistency of these changes. I've found a few minor inconsistencies in JSON tags where omitempty is missing for new pointer fields, which could lead to null values in the JSON output instead of omitting the field. Overall, this is a solid improvement to the API models.

api/model/api/base/http.go
Comment on lines +67 to +91
// +kubebuilder:validation:Optional (default: true)
KeepAlive *bool `json:"keepAlive"`
// +kubebuilder:validation:Optional
// +kubebuilder:default:=true
KeepAlive bool `json:"keepAlive"`
// +kubebuilder:validation:Optional
// +kubebuilder:default:=30000
// Should keep alive be used for the HTTP connection ?
KeepAliveTimeout uint64 `json:"keepAliveTimeout"`
// Should keep alive be used for the HTTP connection ? (default: 30000)
KeepAliveTimeout *uint64 `json:"keepAliveTimeout"`
// Read timeout
// +kubebuilder:validation:Optional
ReadTimeout *uint64 `json:"readTimeout,omitempty"`
// +kubebuilder:default:=false
// Should HTTP/1.1 pipelining be used for the connection or not ?
Pipelining bool `json:"pipelining"`
// Should HTTP/1.1 pipelining be used for the connection or not ? (default: false)
Pipelining *bool `json:"pipelining"`
// HTTP max concurrent connections
// +kubebuilder:validation:Optional
MaxConcurrentConnections *int `json:"maxConcurrentConnections,omitempty"`
// +kubebuilder:default:=false
// Should compression be used or not ?
UseCompression bool `json:"useCompression"`
// Should compression be used or not ? (default: false)
UseCompression *bool `json:"useCompression"`
// +kubebuilder:validation:Optional
// +kubebuilder:default:=false
// Propagate Client Accept-Encoding header
PropagateClientAcceptEncoding bool `json:"propagateClientAcceptEncoding"`
// +kubebuilder:default:=false
// Propagate Client Accept-Encoding header (default: false)
PropagateClientAcceptEncoding *bool `json:"propagateClientAcceptEncoding"`
// Should HTTP redirects be followed or not ?
FollowRedirects bool `json:"followRedirects"`
// +kubebuilder:validation:Optional
// +kubebuilder:default:=true
// Should HTTP/2 clear text upgrade be used or not ?
ClearTextUpgrade bool `json:"clearTextUpgrade"`
// +kubebuilder:default:=HTTP_1_1
// HTTP Protocol Version (Possible values Http1 or Http2)
ProtocolVersion ProtocolVersion `json:"version,omitempty"`
FollowRedirects *bool `json:"followRedirects"`
// +kubebuilder:validation:Optional
// Should HTTP/2 clear text upgrade be used or not ? (default: true)
ClearTextUpgrade *bool `json:"clearTextUpgrade"`
// HTTP Protocol Version (Possible values Http1 or Http2) (default: HTTP_1_1)
ProtocolVersion *ProtocolVersion `json:"version,omitempty"`
Copy link

Choose a reason for hiding this comment

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

medium

To ensure consistency and avoid sending null values in the JSON output for unset fields, the omitempty option should be added to the json tag for all new pointer fields in this struct. This applies to KeepAlive, KeepAliveTimeout, Pipelining, UseCompression, PropagateClientAcceptEncoding, FollowRedirects, and ClearTextUpgrade.

	// +kubebuilder:validation:Optional (default: true)
	KeepAlive *bool `json:"keepAlive,omitempty"`
	// +kubebuilder:validation:Optional
	// Should keep alive be used for the HTTP connection ? (default: 30000)
	KeepAliveTimeout *uint64 `json:"keepAliveTimeout,omitempty"`
	// Read timeout
	// +kubebuilder:validation:Optional
	ReadTimeout *uint64 `json:"readTimeout,omitempty"`
	// Should HTTP/1.1 pipelining be used for the connection or not ? (default: false)
	Pipelining *bool `json:"pipelining,omitempty"`
	// HTTP max concurrent connections
	// +kubebuilder:validation:Optional
	MaxConcurrentConnections *int `json:"maxConcurrentConnections,omitempty"`
	// Should compression be used or not ? (default: false)
	UseCompression *bool `json:"useCompression,omitempty"`
	// +kubebuilder:validation:Optional
	// Propagate Client Accept-Encoding header (default: false)
	PropagateClientAcceptEncoding *bool `json:"propagateClientAcceptEncoding,omitempty"`
	// Should HTTP redirects be followed or not ?
	FollowRedirects *bool `json:"followRedirects,omitempty"`
	// +kubebuilder:validation:Optional
	// Should HTTP/2 clear text upgrade be used or not ? (default: true)
	ClearTextUpgrade *bool `json:"clearTextUpgrade,omitempty"`
	// HTTP Protocol Version (Possible values Http1 or Http2) (default: HTTP_1_1)
	ProtocolVersion *ProtocolVersion `json:"version,omitempty"`

api/model/api/base/resource.go
// Is resource enabled or not?
Enabled bool `json:"enabled"`
// Is resource enabled or not? (default: true)
Enabled *bool `json:"enabled"`
Copy link

Choose a reason for hiding this comment

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

medium

To ensure consistency with other fields and avoid sending null for unset values, omitempty should be added to the json tag.

Suggested change
Enabled *bool `json:"enabled"`
Enabled *bool `json:"enabled,omitempty"`

api/model/api/v2/flow.go
// Indicate if this flow is enabled or disabled
Enabled bool `json:"enabled"`
// Indicate if this flow is enabled or disabled (default: true)
Enabled *bool `json:"enabled"`
Copy link

Choose a reason for hiding this comment

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

medium

To ensure consistency with other fields and avoid sending null for unset values, omitempty should be added to the json tag.

Suggested change
Enabled *bool `json:"enabled"`
Enabled *bool `json:"enabled,omitempty"`

api/model/api/v2/services.go
// Is service enabled or not?
Enabled bool `json:"enabled"`
// Is service enabled or not? (default: false)
Enabled *bool `json:"enabled"`
Copy link

Choose a reason for hiding this comment

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

medium

To ensure consistency with other fields and avoid sending null for unset values, omitempty should be added to the json tag.

Suggested change
Enabled *bool `json:"enabled"`
Enabled *bool `json:"enabled,omitempty"`

@sonarqubecloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
71.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@kamiiiel