Skip to content

Add support for favorite/pin dags to dashboard #51264

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.

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

PNL0
Copy link

@PNL0 PNL0 commented May 31, 2025

The DAGs can be favorited in the DAGs list menu and are displayed in the Dashboard for easier access.
Information about the favorited DAGs is stored by user in the database and retrieved with API calls.

Dashboard view with 3 favorited DAGs (2 of them with previous DAG runs):

image

DAGs list menu view with some of the DAGs favorited:

image

cc @pierrejeambrun @bbovenzi

Closes: #45207

Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Looks good in general in my view. Besides th i18n also DB migration is missing allowing to upgrade existing setups.

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 PR, LGMT.

FYI: https://github.com/apache/airflow/blob/main/contributing-docs/14_metadata_database_updates.rst for fixing database migrations

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!

A few comments

@bbovenzi bbovenzi added this to the Airflow 3.1.0 milestone Jun 4, 2025
@PNL0
Copy link
Author

PNL0 commented Jun 5, 2025

Looks good in general in my view. Besides th i18n also DB migration is missing allowing to upgrade existing setups.

Thanks for the PR, LGMT.

FYI: https://github.com/apache/airflow/blob/main/contributing-docs/14_metadata_database_updates.rst for fixing database migrations

The DB migration should be present now. Let me know if anything is still missing or incomplete.

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, thanks for all the hard work.

A few minor suggestions, but looking good overall.

@PNL0
Copy link
Author

PNL0 commented Jun 18, 2025

Hello @pierrejeambrun @bbovenzi,

I just pushed a new commit that corrects the majority of the problems you pointed out.
Let me know if everything is as expected and if I didn't miss any of your requests (except the 2 not resolved in the conversation).

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!

We're getting closer, a few suggestions

@PNL0
Copy link
Author

PNL0 commented Jun 26, 2025

Hello @bbovenzi,

I just pushed some changes that should cover your requests.
Let me know if everything is ok.

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.

Looks like we need a rebase too

@PNL0
Copy link
Author

PNL0 commented Jun 27, 2025

Hello @bbovenzi,

The rebase is done.

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.

This is looking really promising, and I think users will love it.

A few suggestions and things to fix before we can merge

We need to fix it for FabAuthManager. If you run breeze with --auth-manager FabAuthManager you will see that the server returns error 500. (apparently we need to cast the user_id at some point because SQLAchemy is not happy about it)

@PNL0
Copy link
Author

PNL0 commented Jun 30, 2025

Hello @pierrejeambrun,

I just pushed some changes that should resolve the issues you pointed out.
I did a rebase to fix some conflicts, but you can see the changes in the last commit.

PNL0 and others added 14 commits June 30, 2025 15:08
Co-authored-by: Mariana Isabel Marçal Santana <[email protected]>
The favorited DAGs by an user, are stored persistently in the database.

Co-authored-by: Mariana Isabel Marçal Santana <[email protected]>
Fix deleted DAGs being shown as favorites.
Review the display structure of the favorite DAGs in the dashboard.

Co-authored-by: Mariana Isabel Marçal Santana <[email protected]>
Co-authored-by: Mariana Isabel Marçal Santana <[email protected]>
Co-authored-by: Mariana Isabel Marçal Santana <[email protected]>
Co-authored-by: Mariana Isabel Marçal Santana <[email protected]>
Create new tests for the favorite/unfavorite endpoints.
Add favorite button to the dag header. Other small fixes.

Co-authored-by: Mariana Isabel Marçal Santana <[email protected]>
Co-authored-by: Mariana Isabel Marçal Santana <[email protected]>
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.

CI is failing for compat test because "DagFavorite" does not exist in older airflow versions. (provider compat tests)

In the test when we clean the table for dag favorite, we need to make that conditional. If we are on old airflow version don't try to clear that non existing table, if we are on a version >= 3.1.0 we know the table exists so we can clear it.

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.

Super cool! 🎉

Thanks for the hard work.

We just need to fix the small backcompat providers tests and we should be ready to merge.

@pierrejeambrun
Copy link
Member

#protm

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.

lgtm whenever CI is green

Looks like the codegen API files are being formatted when they shouldn't be

@pierrejeambrun
Copy link
Member

pierrejeambrun commented Jul 1, 2025

Looks like the codegen API files are being formatted when they shouldn't be

Right, that's odd.

Might need to rebase we're pretty behind main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:airflow-ctl area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for favorite/pin dags to dashboard.
6 participants