feat: Add limit to GetTimeline query #375
Conversation
| @@ -151,6 +152,7 @@ message TimelineQuery { | |||
| } | |||
|
|
|||
| repeated Filter filters = 1; | |||
| int32 limit = 2; | |||
There was a problem hiding this comment.
I tried making this optional but for some reason, it was always nil even if I passed it in a GetTimeline request from the console. I couldn't figure out how to resolve that so I removed the optionality. Not sure what's up there.
| if limit != nil { | ||
| q += fmt.Sprintf(" LIMIT %d", *limit) | ||
| } |
There was a problem hiding this comment.
Is this the right approach to this? I mostly needed limit for pagination. I could also add a Filter but it seemed like it might be more clear and outside of the filter flow, but curious what you think @alecthomas
There was a problem hiding this comment.
Looks fine to me, except for the use of *int
backend/controller/console.go
Outdated
| // Get 1 more than the requested limit to determine if there are more results. | ||
| limitPlusOne := limit + 1 | ||
|
|
||
| results, err := c.dal.QueryEvents(ctx, &limitPlusOne, query...) |
There was a problem hiding this comment.
This should be a types.Option or just an int where 0 is treated as missing
| @@ -180,7 +180,7 @@ type eventRow struct { | |||
| RequestKey types.Option[model.IngressRequestKey] | |||
| } | |||
|
|
|||
| func (d *DAL) QueryEvents(ctx context.Context, filters ...EventFilter) ([]Event, error) { | |||
| func (d *DAL) QueryEvents(ctx context.Context, limit *int, filters ...EventFilter) ([]Event, error) { | |||
There was a problem hiding this comment.
Actually, if this truly is optional, it should just be a filter function like all the others.
There was a problem hiding this comment.
ok maybe I'll just go that route then. I wasn't sure if there was a common pattern here I should follow but it sounds like Filters might be the way to go
No description provided.