chore(refactor): create timeline package in controller #2674
Conversation
wesbillman
added
the
run-all
wesbillman
requested review from
a team,
jonathanj-square and
deniseli
and removed request for
a team
| 'log', | ||
| sqlc.arg('payload') | ||
| ); | ||
|
|
||
| -- name: InsertTimelineDeploymentCreatedEvent :exec |
There was a problem hiding this comment.
I wanted to remove these as well, but they are used in a transaction in the controller/dal and I didn't want to break that functionality for now. We could insert the timeline event after the transaction as well, which might be cleaner, but would slightly decouple the event from the action, just slightly though :)
There was a problem hiding this comment.
Definitely pull them out into a separate .sql file, and just add them to the controller/dal sql gen, like here.
There was a problem hiding this comment.
Ok I think I did this correctly, but please lemme know if I'm off the rails :)
| ) | ||
|
|
||
| type Service struct { | ||
| dal dal.DAL |
There was a problem hiding this comment.
I think this should just be a pointer like it is everywhere else.
| Response optional.Option[*ftlv1.CallResponse] | ||
| CallError optional.Option[error] |
There was a problem hiding this comment.
Could use an either here, which might make it a bit clearer:
| Response optional.Option[*ftlv1.CallResponse] | |
| CallError optional.Option[error] | |
| Response either.Either[*ftlv1.CallResponse, error] |
There was a problem hiding this comment.
oh neat! I didn't know about either
| return nil | ||
| } | ||
|
|
||
| func (s *Service) QueryTimeline(ctx context.Context, limit int, filters ...dal.TimelineFilter) ([]dal.TimelineEvent, error) { |
There was a problem hiding this comment.
Ideally I think we wouldn't be exposing dal types directly here, but in practice I think that would be difficult to achieve.
There was a problem hiding this comment.
Yeah I agree. I was torn here since it would be a bit of duplication. maybe as I add more to the timeline it will make sense to fully decouple. I'll keep an eye on it
wesbillman
force-pushed
the
create-timeline-package-in-controller
branch
from
c0a146c to
df981d8
Compare
sqlc.yaml
Outdated
| type: "optional.Option[time.Time]" | ||
| - db_type: "pg_catalog.interval" | ||
| go_type: "github.com/TBD54566975/ftl/backend/controller/sql/sqltypes.Duration" | ||
| - db_type: "pg_catalog.interval" |
There was a problem hiding this comment.
We don't want to duplicate these types, just keep all the type mappings in the root overrides.
There was a problem hiding this comment.
Hmm I must be doing something weird because if I don't have these it seems like the code won't build anymore. Seems like stuff generates like this:
type InsertTimelineCallEventParams struct {
DeploymentKey interface{}
RequestKey sql.NullString
ParentRequestKey sql.NullString
TimeStamp time.Time
SourceModule sql.NullString
SourceVerb sql.NullString
DestModule string
DestVerb string
Payload encryption.EncryptedTimelineColumn
}Instead of optional.Option[string] for example. Maybe I need another change for the sqlc config
There was a problem hiding this comment.
I dunno. I deleted it, merged it, and everything compiled fine... 🤷♂️
alecthomas
force-pushed
the
create-timeline-package-in-controller
branch
from
61abf6d to
6bc23c1
Compare
alecthomas
force-pushed
the
create-timeline-package-in-controller
branch
from
a61c517 to
dd41626
Compare
alecthomas
force-pushed
the
create-timeline-package-in-controller
branch
from
dd41626 to
63c99e0
Compare
Shouldn't change any functionality just creating a new package for the timeline since it's has a bunch of code and we'll be adding many more event types in the near future.