Conversation
jason810496
requested review from
bbovenzi,
ephraimbuddy,
jscheffl,
pierrejeambrun and
ryanahamilton
as code owners
boring-cyborg
bot
added
area:API
|
I reviewed the other PR on which is based this one. I will do an in-depth review here when the other one is updated and merge because it will most likely imply changes here as well. |
jason810496
force-pushed
the
feature/AIP-84/add-get-event-logs
branch
from
169e45e to
64d20a4
Compare
|
Fix common issue in #43406 ( should add |
pierrejeambrun
added
the
legacy api
pierrejeambrun
left a comment
pierrejeambrun
left a comment
There was a problem hiding this comment.
I would take the work on FilterParam out of this particular PR because it's impact is much broader and we should update other endpoints if we want to include such new logic.
I would just use the base_query to .filter a few thing, and handle query parameters before, after in the same way as we have right now with new params.
Then we can merge this and you can start working on a larger refactoring to include FilterParam, taking the time to make the dependency injection work for it and also refactor other endpoints to follow this new construct.
|
Sure, I will separate |
jason810496
force-pushed
the
feature/AIP-84/add-get-event-logs
branch
4 times, most recently
from
ab485b3 to
aeed230
Compare
|
Hi @pierrejeambrun, I believe this PR is ready to be merged. Afterward, I’ll start working on PR to refactor SortParam and FilterParam. |
pierrejeambrun
left a comment
pierrejeambrun
left a comment
There was a problem hiding this comment.
Ready to merge, need rebasing to solve conflicts
For Range filtering, you can take a look at which implements some helper code, you might need to re-use some of it for your refactoring: |
pierrejeambrun
force-pushed
the
feature/AIP-84/add-get-event-logs
branch
from
aeed230 to
5a8b72d
Compare
|
I just rebased the branch, waiting for a green CI to merge. |
pierrejeambrun
force-pushed
the
feature/AIP-84/add-get-event-logs
branch
from
5a8b72d to
9ef7586
Compare
|
Thanks for helping resolve the conflict. |
* AIP-84 Get Event Logs * fix: add http execption docs for router * refactor: remove `FilterParam` out of this PR
2 participants
Closes: #43326
Related: #42370
Related discussion mentioned at #43204 (comment)