AIP-84: Add UI batch recent dag runs endpoint#43204
Conversation
0b552c7 to
5252d7a
Compare
Refactor:Used |
There was a problem hiding this comment.
I am not sure I understand the OptionalModel. Why do those DAGRuns and DAGs omit fields in the private response compare to the public one ?
I would assume that all of the public fields are also defined and here in the private endpoint therefore removing the need for that OptionalModel class. Did you encounter a specific issue ?
|
I think for this UI, the endpoint doesn't need to respond with so much information (compared with Instead of marking the fields that aren't needed in this endpoint as optional in |
|
I think we can just return the whole information and don't bother filtering things out. For the front-end it is easier because you only manipulate 1 type for the returned DAG, DagRun, etc... You do not have different representation depending on the endpoint (private/public), which makes things easier to work with (fill/cast etc..).
|
5252d7a to
47c69b9
Compare
|
Resolved: Made this UI endpoint more consistent with the public endpoint by removing |
|
@jason810496 Nice, I just added a small commit on top to make a few adjustment, let me know what you think, I believe we can merge then. |
|
Hi @pierrejeambrun, the changes look good to me, but I have another question: When should I utilize a parameter class (the classes declared in For example, in the Event Log endpoints, the query should leverage |
The idea is if the parameter (query or path) is meant to be re-used in other endpoints and implement a common db operation (filtering, sorting, matching, anything), then it should go into If like here it is a very specific one, in terms of parameters (a secondary limit index operating on nested ressources) in terms of naming "dag_runs_limit", and in terms of implementation ( |
cf18fd3 to
e20cd91
Compare
e20cd91 to
c58a7e7
Compare
* AIP-84 | add UI batch recent dag runs endpoint * AIP-84 | add UI batch recent dag runs * refactor: use public serializer for ui batch recent runs * fix: add trigger_by for dag run in public test_dag * refactor: use DAGRunResponse from public endpoint * Update code review --------- Co-authored-by: pierrejeambrun <[email protected]>
closes: #42933
Same filter with public
get_dagsendpoint, but remove order_by params.