PLATFORM-9039: webhooks reliability #98
Conversation
korzepadawid
changed the base branch from
fandom_master_1_0_0
to
PLATFORM-8899-CR
korzepadawid
changed the title
korzepadawid
force-pushed
the
PLATFORM-9039-webhooks-reliability
branch
from
b9fdd63 to
744cbfd
Compare
driver/registry_default.go
Outdated
| m.rc[name] = rc | ||
| m.rwl.Unlock() |
There was a problem hiding this comment.
there was a data race before
There was a problem hiding this comment.
a map in Go is not thread-safe
There was a problem hiding this comment.
nit: Could we use https://pkg.go.dev/sync#Map instead of manual synchronization?
| @@ -540,17 +538,28 @@ func (e *WebHook) execute(ctx context.Context, data *templateContext) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func parseWebhookResponse(resp *http.Response, id *identity.Identity) (err error) { | |||
| func (e *WebHook) parseWebhookResponse(resp *http.Response, id *identity.Identity) (err error) { | |||
There was a problem hiding this comment.
let's try to push it to the upstream
There was a problem hiding this comment.
otherwise, we won't have an access to the logger
There was a problem hiding this comment.
the logger is only available through e.deps
| @@ -934,8 +936,8 @@ func TestWebHooks(t *testing.T) { | |||
| // error. | |||
|
|
|||
| var wg sync.WaitGroup | |||
| wg.Add(3) // HTTP client does 3 attempts | |||
There was a problem hiding this comment.
that's no longer true assuming this https://github.com/Wikia/kratos/pull/97/files#r1479563845
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## PLATFORM-8899-CR #98 +/- ##
===================================================
Coverage ? 76.58%
===================================================
Files ? 331
Lines ? 22055
Branches ? 0
===================================================
Hits ? 16891
Misses ? 3867
Partials ? 1297 ☔ View full report in Codecov by Sentry. |
| // 204: emptyResponse | ||
| // 404: jsonError | ||
| // 500: jsonError | ||
| func (h *Handler) deleteMultifactorCredential(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { |
There was a problem hiding this comment.
Related issue(s)
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further Comments