-
Notifications
You must be signed in to change notification settings - Fork 1
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
reconciler: Fix Status JSON marshalling #63
Conversation
43587d4 broke JSON marshalling of Status by introducing a separate statusJSON type and Unmarshal{JSON,YAML} methods, but didn't introduce the Marshal{JSON,YAML} methods, which caused dictionary key mismatches as Status struct was used to marshal instead of statusJSON. Fix this by removing the whole statusJSON construct and add json and yaml tags to Status. There is no need for filling in the `id` field when unmarshalling Status as this is completely internal to the reconciler. Fixes: 43587d4 ("reconciler: Implement tests using scripttest") Signed-off-by: Jussi Maki <[email protected]>
|
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
Kind StatusKind | ||
UpdatedAt time.Time | ||
Error string | ||
Kind StatusKind `json:"kind" yaml:"kind"` |
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.
do you even need these struct tags?
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.
Not necessarily. My thinking is that this makes the struct nicer to use in scripttest.
43587d4 broke JSON marshalling of Status by introducing a separate statusJSON type and Unmarshal{JSON,YAML} methods, but didn't introduce the Marshal{JSON,YAML} methods, which caused dictionary key mismatches as Status struct was used to marshal instead of statusJSON.
Fix this by removing the whole statusJSON construct and add json and yaml tags to Status. There is no need for filling in the
id
field when unmarshalling Status as this is completely internal to the reconciler.Fixes: 43587d4 ("reconciler: Implement tests using scripttest")