chore(refactor): add ingress event type and refactor timeline #2719
Conversation
wesbillman
requested review from
a team,
stuartwdouglas and
deniseli
and removed request for
a team
wesbillman
force-pushed
the
add-ingress-event-type-and-refactor
branch
2 times, most recently
from
e92ecb3 to
a1ff7cc
Compare
| RequestKey model.RequestKey | ||
| } | ||
|
|
||
| // The internal JSON payload of an ingress event. |
There was a problem hiding this comment.
nittiest of nits: some of the json structs have comments and others don't. Did you want to remove the extant comments or add a comment wherever they're missing?
There was a problem hiding this comment.
Haha I'm just random 😂 I'll make sure they all have them 👍
| Stack optional.Option[string] `json:"stack,omitempty"` | ||
| } | ||
|
|
||
| type Call struct { |
There was a problem hiding this comment.
Why do we need both Call and CallEvent?
There was a problem hiding this comment.
The idea here is the Call is what's used as the external API to insert calls into the db. And CallEvents are what are returned from the query. Maybe there's a better name for this 🤔
There was a problem hiding this comment.
Makes sense but I would embed Call into CallEvent so you're not duplicating fields.
There was a problem hiding this comment.
Awesome! Will do in the next PR. I might have some questions about how to do this correctly :)
| (CASE | ||
| WHEN sqlc.narg('parent_request_key')::TEXT IS NULL THEN NULL | ||
| ELSE (SELECT id FROM requests ir WHERE ir.key = sqlc.narg('parent_request_key')::TEXT) | ||
| END), | ||
| sqlc.arg('time_stamp')::TIMESTAMPTZ, | ||
| 'ingress', | ||
| sqlc.narg('source')::TEXT, | ||
| sqlc.narg('destination_module')::TEXT, | ||
| sqlc.arg('ingress_type')::TEXT, | ||
| sqlc.narg('http_method')::TEXT, | ||
| sqlc.arg('payload') |
There was a problem hiding this comment.
How did you want to handle the rest of these args, since only deployment_key and request_key are populated in events_ingress.go?
There was a problem hiding this comment.
Yeah you can ignore this for now. I'm going to actually populate the ingress event right after this PR merges, but I need to think through the exact fields a bit and didn't want to keep adding to this mega PR
| } | ||
| // TimelineEvent types. | ||
| // | ||
| //sumtype:decl |
There was a problem hiding this comment.
Does this do anything outside of a module? :o
There was a problem hiding this comment.
I think it's for the sumtype tool in go to make this interface as a sumtype.
| if err := d.encryption.DecryptJSON(&row.Payload, &jsonPayload); err != nil { | ||
| return nil, fmt.Errorf("failed to decrypt call event: %w", err) | ||
| if err := s.encryption.DecryptJSON(&row.Payload, &jsonPayload); err != nil { | ||
| return nil, fmt.Errorf("failed to decrypt deployment created event: %w", err) |
wesbillman
force-pushed
the
add-ingress-event-type-and-refactor
branch
from
2d05edb to
cd4b33f
Compare
| Stack optional.Option[string] `json:"stack,omitempty"` | ||
| } | ||
|
|
||
| type Call struct { |
There was a problem hiding this comment.
Makes sense but I would embed Call into CallEvent so you're not duplicating fields.
| Error optional.Option[string] `json:"error,omitempty"` | ||
| } | ||
|
|
||
| type Log struct { |
There was a problem hiding this comment.
Same thing here, and probably with all the events potentially? I'd embed the struct.
This is mostly the refactor and setting up for the new
ingressevents. I'm pushing this up first because it will get quite large once I addingressand a bit more refactoring. :)Actual ingress events will come next.