-
Notifications
You must be signed in to change notification settings - Fork 40
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(*): avoid setting nil for missing fields #463
Conversation
4fe0227
to
0492afb
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #463 +/- ##
==========================================
+ Coverage 59.85% 59.94% +0.08%
==========================================
Files 71 71
Lines 4449 4449
==========================================
+ Hits 2663 2667 +4
+ Misses 1167 1165 -2
+ Partials 619 617 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
// if no default exists, remove the field | ||
delete(res, fname) |
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 is the actual change
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.
There are some plugins that require explicit null
here.
We need to allow users to explicitly specify a null
value, but agree that we should not populate null
from defaults
not sure why the linter got mad |
https://github.com/Kong/go-kong/actions/runs/10493904848/job/29068947573?pr=463 |
For visibility: I do not fully understand why there is even this Now if tool itself needs these to be filled (e.g. for easier diffing or something), it is another question, but that should not affect what is sent to wire to Kong API (provided that Koko API works the same). This of course means that the tool should also never use That said, we can first stop filling |
Stop filling default nulls, you mean? Active null should still be accepted |
yes @mheap that is correct, this change is intended to only apply to auto-filled defaults |
upon filling defaults for the configuration, the existing logic was setting `nil` on missing fields, but in Kong Gateway, explicit `nil` are not treated as a synonym of "missing field". This change removes any missing fields from the configuration instead of explicitly setting such fields as `nil`.
0492afb
to
6dbf5df
Compare
Note: this appears to be breaking the decK diff output (where now all nulls show up as diffs) - probably expected given the change here, because of this, so we might need to do it differently (?), e.g. like @bungle says, by preventing default to be filled at all when the config is sent to Kong (and keep the behavior unchanged for diffs). I suspect that change must be done |
second attempt: Kong/go-database-reconciler#133 |
when filling defaults, the existing logic sets
nil
on missing fields, but in Kong Gateway explicitnil
s result in the corresponding fields to be unset, causing problems such as: KAG-5157This change removes any unset fields that don't have a default from the configuration instead of explicitly setting them to
nil
.@reviewers
: could this be a breaking change? I tested it in decK and it seems to work but I'm worried about other projects using this lib. An alternative would be creating another utils function with the new behavior (which I think would require updating at leastdeck
,go-database-reconciler
and probably kubernetes-ingress-controller), but if we're fairly confident the func is only used while interacting with decK then it might be considered safe enough.Searching occurrences.
For more details see: KAG-5210