fix(*): avoid setting nil for missing fields #463
Conversation
samugi
force-pushed
the
fix/avoid-setting-nil-for-missing-fields
branch
3 times, most recently
from
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.
this is the actual change
There was a problem hiding this comment.
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`.
samugi
force-pushed
the
fix/avoid-setting-nil-for-missing-fields
branch
from
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
nilon missing fields, but in Kong Gateway explicitnils 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-reconcilerand 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