Skip to content

AIP-79 Generate assets for Flask application in FAB provider#44744

Merged
vincbeck merged 1 commit into
apache:mainfrom
aws-mwaa:vincbeck/compile_assets
Dec 18, 2024
Merged

AIP-79 Generate assets for Flask application in FAB provider#44744
vincbeck merged 1 commit into
apache:mainfrom
aws-mwaa:vincbeck/compile_assets

Conversation

@vincbeck

@vincbeck vincbeck commented Dec 6, 2024

Copy link
Copy Markdown
Contributor

Follow up of #44464.

In #44464, we copied a minimal version of the Flask application in FAB provider to support Airflow 2 plugins. This Flask application required assets (like the Flask application in core).

In this PR I update the script compile_www_assets.py to also generate the assets for the Flash application in FAB provider.

For later: when the current UI is gone, we'll be able to stop generating the assets for the main Flask application in core (then maybe move the script to FAB provider?).


^ 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 newsfragments.

@jedcunningham jedcunningham left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Overall, lgtm. Do we need to add a step to run this into the provider release process for FAB?

@vincbeck

vincbeck commented Dec 6, 2024

Copy link
Copy Markdown
Contributor Author

Overall, lgtm. Do we need to add a step to run this into the provider release process for FAB?

I think so. Do you know where is this provider release process?

@vincbeck

vincbeck commented Dec 6, 2024

Copy link
Copy Markdown
Contributor Author

I just had a double thought. Would not it be better to copy paste this script in FAB provider and generate only the assets for FAB provider in this script, update the release process of FAB provider to run this script? That way, when we remove the entire Flash application from core, we can safely remove the script from core?

@jedcunningham

Copy link
Copy Markdown
Member

Oh, that probably is a better idea.

These are the docs for the release process: https://github.com/apache/airflow/blob/main/dev/README_RELEASE_PROVIDER_PACKAGES.md

@vincbeck

vincbeck commented Dec 9, 2024

Copy link
Copy Markdown
Contributor Author

Oh, that probably is a better idea.

These are the docs for the release process: https://github.com/apache/airflow/blob/main/dev/README_RELEASE_PROVIDER_PACKAGES.md

Cool :) I'll update the PR

@vincbeck

vincbeck commented Dec 9, 2024

Copy link
Copy Markdown
Contributor Author

Oh, that probably is a better idea.
These are the docs for the release process: https://github.com/apache/airflow/blob/main/dev/README_RELEASE_PROVIDER_PACKAGES.md

Cool :) I'll update the PR

I just realized, the release should not be updated because the assets are not released. It should be in the install process

@vincbeck

vincbeck commented Dec 9, 2024

Copy link
Copy Markdown
Contributor Author

Also it seems there is already some plumbing and automations set up to generate the assets automatically when creating a virtual env with hatch so I think I'll keep everything in that file. That will simplify things. I added a test in the script to do it only if the fab provider package is installed

@vincbeck
vincbeck force-pushed the vincbeck/compile_assets branch from 448417e to 92e7dc6 Compare December 9, 2024 16:26
@vincbeck
vincbeck force-pushed the vincbeck/compile_assets branch from 92e7dc6 to f2b0d43 Compare December 9, 2024 20:26
@vincbeck

Copy link
Copy Markdown
Contributor Author

Also it seems there is already some plumbing and automations set up to generate the assets automatically when creating a virtual env with hatch so I think I'll keep everything in that file. That will simplify things. I added a test in the script to do it only if the fab provider package is installed

@jedcunningham WDYT?

@vincbeck
vincbeck requested a review from Copilot December 11, 2024 16:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no suggestions.

@vincbeck
vincbeck merged commit ba49469 into apache:main Dec 18, 2024
@vincbeck
vincbeck deleted the vincbeck/compile_assets branch December 18, 2024 18:40


def is_fab_provider_installed() -> bool:
return importlib.util.find_spec("airflow.providers.fab") is not None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ups, this change in this function broke main :-(
https://github.com/apache/airflow/actions/runs/12399970802/job/34616478712

I assume we need to find another method to check if FAB is installed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Propose to fix via #45058

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants