-
Notifications
You must be signed in to change notification settings - Fork 46
Modernize Go code with latest style conventions #646
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
modules/common/route/types.go
Outdated
| // is allowed, and it will be defaulted to Service. If the weight field (0-256 default 100) | ||
| // is set to zero, no traffic will be sent to this backend. | ||
| To TargetReference `json:"to,omitempty" protobuf:"bytes,3,opt,name=to"` | ||
| To TargetReference `json:"to" protobuf:"bytes,3,opt,name=to"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems in line with [1]. @stuggi do you see any potential problem with this?
[1] https://github.com/openshift/api/blob/master/route/v1/types.go#L132
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this copy we intentionally set everything to optional + omitempty for not showing all in the override spec + e.g. we have to remove required as we want to be able to override only a subset without the need to provide any required parameter, as in the org we copied it from
| // +kubebuilder:validation:optional | ||
| // +operator-sdk:csv:customresourcedefinitions:type=spec | ||
| // API tls type which encapsulates for API services | ||
| API APIService `json:"api,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect it removes omitempty because we have // +kubebuilder:validation:optional. @stuggi do we want to keep the original version? (this comment applies to L112 as well).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afaik, for a struct like APIService, its zero value is an instance where all its fields are empty (e.g., nil pointers, empty strings, 0 for numbers). so I think we want omitempty to not show the defaults of the sub struct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack, in general I'll restore omitempty for both APIService and TargetReference, so we do not risk to have any unwanted change to the CRD that might result in unwanted behavior due to a change in the existing CRs.
From what I can see in a lab, all my deployed services have:
tls:
api:
internal: {}
public: {}which I think means that omitempty at APIService level is not useful because the underlying fields:
// APIService - API tls type which encapsulates for API services
type APIService struct {
// +kubebuilder:validation:optional
// +operator-sdk:csv:customresourcedefinitions:type=spec
// Public GenericService - holds the secret for the public endpoint
Public GenericService `json:"public"`
// +kubebuilder:validation:optional
// +operator-sdk:csv:customresourcedefinitions:type=spec
// Internal GenericService - holds the secret for the internal endpoint
Internal GenericService `json:"internal"`
}have not omitempty, and therefore are always rendered in the resulting CR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To TargetReference `json:"to" protobuf:"bytes,3,opt,name=to", instead, is different, because omitempty in that case takes effect because the underlying fields have omitempty as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, I restored the omitempty tags so we don't risk to get any side effect due to this change.
Applied automated modernization using gopls modernize tool to update:
- Replace interface{} with any type alias (Go 1.18+)
- Update range loops to use idiomatic range syntax
- Replace fmt.Sprintf with fmt.Appendf where appropriate
These changes improve code readability and align with current Go best
practices.
Co-Authored-By: Claude <[email protected]>
Signed-off-by: Francesco Pantano <[email protected]>
abays
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Applied automated modernization using gopls modernize tool to update:
These changes improve code readability and align with current Go best practices.
Co-Authored-By: Claude [email protected]