Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

AIP-84 Get Event Logs #43407

Merged
merged 3 commits into from
Nov 5, 2024
Merged

AIP-84 Get Event Logs #43407

merged 3 commits into from
Nov 5, 2024

Conversation

jason810496
Copy link
Contributor

@jason810496 jason810496 commented Oct 27, 2024 •
edited by pierrejeambrun
Loading

Closes: #43326
Related: #42370

Related discussion mentioned at #43204 (comment)

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers. labels Oct 27, 2024
@pierrejeambrun
Copy link
Member

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 jason810496 force-pushed the feature/AIP-84/add-get-event-logs branch from 169e45e to 64d20a4 Compare October 30, 2024 03:23
@jason810496
Copy link
Contributor Author

Fix common issue in #43406 ( should add create_openapi_http_exception_doc at router ).

@pierrejeambrun pierrejeambrun added the legacy api Whether legacy API changes should be allowed in PR label Oct 30, 2024
pierrejeambrun
pierrejeambrun reviewed Oct 30, 2024
View reviewed changes
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

airflow/api_fastapi/common/parameters.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/routes/public/event_logs.py Outdated Show resolved Hide resolved
@jason810496
Copy link
Contributor Author

Sure, I will separate FilterParam from this PR.

@jason810496 jason810496 force-pushed the feature/AIP-84/add-get-event-logs branch 4 times, most recently from ab485b3 to aeed230 Compare November 1, 2024 10:26
@jason810496
Copy link
Contributor Author

Hi @pierrejeambrun, I believe this PR is ready to be merged. Afterward, I’ll start working on PR to refactor SortParam and FilterParam.

pierrejeambrun
pierrejeambrun approved these changes Nov 5, 2024
View reviewed changes
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready to merge, need rebasing to solve conflicts

@pierrejeambrun
Copy link
Member

pierrejeambrun commented Nov 5, 2024 •
edited
Loading

Hi @pierrejeambrun, I believe this PR is ready to be merged. Afterward, I’ll start working on PR to refactor SortParam and FilterParam.

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:
#43642

@pierrejeambrun pierrejeambrun force-pushed the feature/AIP-84/add-get-event-logs branch from aeed230 to 5a8b72d Compare November 5, 2024 08:42
@pierrejeambrun
Copy link
Member

I just rebased the branch, waiting for a green CI to merge.

@pierrejeambrun pierrejeambrun force-pushed the feature/AIP-84/add-get-event-logs branch from 5a8b72d to 9ef7586 Compare November 5, 2024 10:24
@pierrejeambrun pierrejeambrun merged commit 6548d50 into apache:main Nov 5, 2024
56 checks passed
@jason810496
Copy link
Contributor Author

Thanks for helping resolve the conflict.
I’ll take a look at the implementation of range filtering.

This was referenced Nov 12, 2024
Merged
Merged
ellisms pushed a commit to ellisms/airflow that referenced this pull request Nov 13, 2024
@jason810496 @ellisms
AIP-84 Get Event Logs (apache#43407)
37948f2 
* AIP-84 Get Event Logs

* fix: add http execption docs for router

* refactor: remove `FilterParam` out of this PR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierrejeambrun pierrejeambrun pierrejeambrun approved these changes

@bbovenzi bbovenzi Awaiting requested review from bbovenzi bbovenzi is a code owner

@ryanahamilton ryanahamilton Awaiting requested review from ryanahamilton ryanahamilton is a code owner

@jscheffl jscheffl Awaiting requested review from jscheffl jscheffl is a code owner

@ephraimbuddy ephraimbuddy Awaiting requested review from ephraimbuddy ephraimbuddy is a code owner

Assignees
No one assigned
Labels
area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers. legacy api Whether legacy API changes should be allowed in PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AIP-84 Migrate the public endpoint Get Event Logs to FastAPI
2 participants
@jason810496 @pierrejeambrun