Skip to content

Add external logs link #49475

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
Merged
merged 7 commits into from
Apr 24, 2025
Merged

Add external logs link #49475

merged 7 commits into from
Apr 24, 2025

Conversation

@guan404ming
Copy link
Member

@guan404ming guan404ming commented Apr 20, 2025 •
edited
Loading

Related Issue

Closes #47100

How

  1. expose show_external_log_redirect and external_log_name (from task_log_reader) through ui config endpoint
  2. implement ExternalLogLink in Log to handle redirect like AF2
  3. use the show_external_log_redirect and external_log_name to determine whether to render ExternalLogLink
image

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers. labels Apr 20, 2025
@guan404ming guan404ming changed the title feat: support external log AIP-38: support external log Apr 20, 2025
@eladkal eladkal added this to the Airflow 3.1.0 milestone Apr 20, 2025
jason810496
jason810496 reviewed Apr 20, 2025
View reviewed changes
Copy link
Member

@jason810496 jason810496 left a comment

Choose a reason for hiding this comment

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

Thanks @guan404ming for the PR! Looks good overall.

I’ve left comments for some missing detailsL

@guan404ming
Copy link
Member Author

guan404ming commented Apr 20, 2025

Thanks for your reviewing, I've updated the loss data model and test~

jason810496
jason810496 reviewed Apr 21, 2025
View reviewed changes
Copy link
Member

@jason810496 jason810496 left a comment

Choose a reason for hiding this comment

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

Thanks for the change! Looks good to me, only left some nits for route definition and the testing part.

@guan404ming
Copy link
Member Author

guan404ming commented Apr 21, 2025

Thanks for help out, it did make sense to make the structure better!

@guan404ming guan404ming force-pushed the add-external-logs branch 2 times, most recently from d41d97b to 8fb2b59 Compare April 21, 2025 15:54
@guan404ming guan404ming force-pushed the add-external-logs branch 2 times, most recently from fa4b2e9 to 60efdb6 Compare April 22, 2025 01:41
pierrejeambrun
pierrejeambrun reviewed Apr 22, 2025
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.

Nice work.

Just a few nits / suggestion.

@guan404ming
Copy link
Member Author

guan404ming commented Apr 22, 2025

Thanks for reviewing, I've updated my PR~

pierrejeambrun
pierrejeambrun approved these changes Apr 22, 2025
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.

Small nits

jason810496
jason810496 approved these changes Apr 22, 2025
View reviewed changes
Copy link
Member

@jason810496 jason810496 left a comment

Choose a reason for hiding this comment

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

Thanks!

bbovenzi
bbovenzi reviewed Apr 22, 2025
View reviewed changes
Copy link
Contributor

@bbovenzi bbovenzi left a comment

Choose a reason for hiding this comment

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

A few small comments:

@guan404ming
Copy link
Member Author

guan404ming commented Apr 23, 2025

PR updated. Thanks for your reviewing~

@guan404ming guan404ming force-pushed the add-external-logs branch 2 times, most recently from b3f0707 to cebab02 Compare April 23, 2025 14:51
@guan404ming guan404ming requested a review from bbovenzi April 23, 2025 17:34
@bbovenzi bbovenzi changed the title AIP-38: support external log Add external logs link Apr 23, 2025
@bbovenzi
Copy link
Contributor

bbovenzi commented Apr 23, 2025

UI is good with me. But it looks like some python tests are failing

@jason810496 jason810496 merged commit ab037cb into apache:main Apr 24, 2025
96 checks passed
@guan404ming guan404ming deleted the add-external-logs branch April 24, 2025 02:07
prabhusneha pushed a commit to astronomer/airflow that referenced this pull request Apr 25, 2025
@guan404ming @jason810496
@pierrejeambrun @bbovenzi
Add external logs link (apache#49475)
a701660 
* feat: support external log

* test: add test and dataModel

Co-Authored-By: LIU ZHE YOU <[email protected]>

* refactor: test structure

Co-Authored-By: LIU ZHE YOU <[email protected]>

* fix: airflowctl type in static check

* fix: api and ui

Co-authored-by: pierrejeambrun <[email protected]>

* fix: miss rel and coding style

* fix: replace a with `Link`

Co-authored-by: Brent Bovenzi <[email protected]>

---------

Co-authored-by: LIU ZHE YOU <[email protected]>
Co-authored-by: pierrejeambrun <[email protected]>
Co-authored-by: Brent Bovenzi <[email protected]>
@guan404ming guan404ming mentioned this pull request Apr 28, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bbovenzi bbovenzi bbovenzi approved these changes

@bugraoz93 bugraoz93 bugraoz93 approved these changes

@pierrejeambrun pierrejeambrun pierrejeambrun approved these changes

@jason810496 jason810496 jason810496 approved these changes

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

@jscheffl jscheffl Awaiting requested review from jscheffl

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

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

@kaxil kaxil Awaiting requested review from kaxil

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

@shubhamraj-git shubhamraj-git Awaiting requested review from shubhamraj-git shubhamraj-git 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.

Projects

None yet

Milestone

Airflow 3.1.0

Development

Successfully merging this pull request may close these issues.

AIP-38 External logs support

6 participants

@guan404ming @bbovenzi @bugraoz93 @pierrejeambrun @jason810496 @eladkal