-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
fix: application/v1alpha1 add json tag omitzero to status field #22233
base: master
Are you sure you want to change the base?
fix: application/v1alpha1 add json tag omitzero to status field #22233
Conversation
✅ Preview Environment deployed on Bunnyshell
See: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
omitzero was introduced in go 1.24 as a way to allow non-pointer structs to not be marshalled when that struct has a "zero" value this solves the problem that when using ArgoCD as a library, marshalling Applications to json do not include the status field, which would otherwise have to be ignored (if using the app of apps pattern) https://pkg.go.dev/encoding/json#Marshal Signed-off-by: Wes McNamee <[email protected]>
8df029e
to
6cd2af6
Compare
@@ -56,7 +56,7 @@ type Application struct { | |||
metav1.TypeMeta `json:",inline"` | |||
metav1.ObjectMeta `json:"metadata" protobuf:"bytes,1,opt,name=metadata"` | |||
Spec ApplicationSpec `json:"spec" protobuf:"bytes,2,opt,name=spec"` | |||
Status ApplicationStatus `json:"status,omitempty" protobuf:"bytes,3,opt,name=status"` | |||
Status ApplicationStatus `json:"status,omitempty,omitzero" protobuf:"bytes,3,opt,name=status"` |
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.
This syntax isn't still valid since the Go version that Argo CD uses is Go.123.x
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.
You are correct @sivchari, this PR would also need a bump to 1.24 (or create a PR upgrading Golang to 1.24)
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.
@blakepettersson
Originally, I'm trying to upgrade to Go1.23.6 to avoid vulnerability in
#21929
But it seems like a good to upgrade to latest version.
So I do it by opening other PR and above one will be closed.
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.
When #22242 is merged, this PR will pass.
@@ -1517,7 +1517,7 @@ type ComparedTo struct { | |||
// SyncStatus contains information about the currently observed live and desired states of an application | |||
type SyncStatus struct { | |||
// Status is the sync state of the comparison | |||
Status SyncStatusCode `json:"status" protobuf:"bytes,1,opt,name=status,casttype=SyncStatusCode"` | |||
Status SyncStatusCode `json:"status,omitempty" protobuf:"bytes,1,opt,name=status,casttype=SyncStatusCode"` |
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.
You need to update manifets.
Maybe, there is the reason why the e2e are failed.
omitzero was introduced in go 1.24 as a way to allow non-pointer structs to not be marshalled when that struct has a "zero" value this solves the problem that when using ArgoCD as a library, marshalling Applications to json do not include the status field, which would otherwise have to be ignored (if using the app of apps pattern)
https://pkg.go.dev/encoding/json#Marshal
Checklist: