Orchestrator webhook JSON to require "address" field #3354
Conversation
| @@ -16,7 +16,7 @@ import ( | |||
| ) | |||
|
|
|||
| type webhookResponse struct { | |||
| Address string `json:"address,omitempty"` | |||
| Address string `json:"address"` | |||
| Score float32 `json:"score,omitempty"` | |||
There was a problem hiding this comment.
Should we also make score required?
There was a problem hiding this comment.
I'd leave it as omitempty. Would need to analyze more how it's used and if we always need it.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3354 +/- ##
===================================================
+ Coverage 33.72602% 33.73050% +0.00448%
===================================================
Files 141 141
Lines 37434 37432 -2
===================================================
+ Hits 12625 12626 +1
+ Misses 24088 24086 -2
+ Partials 721 720 -1
Continue to review full report in Codecov by Sentry.
|
| @@ -16,7 +16,7 @@ import ( | |||
| ) | |||
|
|
|||
| type webhookResponse struct { | |||
| Address string `json:"address,omitempty"` | |||
| Address string `json:"address"` | |||
| Score float32 `json:"score,omitempty"` | |||
There was a problem hiding this comment.
I'd leave it as omitempty. Would need to analyze more how it's used and if we always need it.
doc/orchwebhook.md
Outdated
| {"address":"https://10.4.3.2:8935"}, | ||
| {"address":"https://10.4.4.3:8935"}, | ||
| {"address":"https://10.4.5.2:8935"} | ||
| {"address":"https://10.4.3.2:8935", "score": 1.0}, |
There was a problem hiding this comment.
Nit: I think you can keep it without scores. But put to you.
pwilczynskiclearcode
force-pushed
the
ENG-2368-orch-webhook-omitempty
branch
from
9503cd2 to
8c1deef
Compare
pwilczynskiclearcode
force-pushed
the
ENG-2368-orch-webhook-omitempty
branch
from
8c1deef to
3dc4b2c
Compare
| @@ -16,7 +16,7 @@ import ( | |||
| ) | |||
|
|
|||
| type webhookResponse struct { | |||
| Address string `json:"address,omitempty"` | |||
| Address string `json:"address"` | |||
There was a problem hiding this comment.
I don't think this does much since omitempty only applies on encoding, not decoding, and the JSON parser will happy deserialize an object that has an empty address field
| @@ -147,12 +147,9 @@ func deserializeWebhookJSON(body []byte) ([]common.OrchestratorLocalInfo, error) | |||
| } | |||
| var infos []common.OrchestratorLocalInfo | |||
| for _, addr := range addrs { | |||
| if addr.Address == "" { | |||
There was a problem hiding this comment.
We probably want to keep this in for resiliency, since the JSON parser will still deserialize objects with empty addresses (unless ParseRequestURI returns an error on empty inputs; can you confirm that?)
I can't figure out the situation why we would accept empty
addressand silently skip it. I think this should be a required field. Also we don't specify default score anywhere on the code or I can't find it