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.

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

Upgrade FAB to 4.3.1 #31203

Merged
merged 2 commits into from
May 13, 2023
Merged

Upgrade FAB to 4.3.1 #31203

merged 2 commits into from
May 13, 2023

Conversation

VVildVVolf
Copy link
Contributor

@VVildVVolf VVildVVolf commented May 11, 2023

Flask-ApplicationBuilder v4.3.0 has a bug with usage Select2 control. It is fixed in v4.3.1. The description is mentioned in issue #31156

please make sure that you review the classes and models in
# airflow/www/fab_security with their upstream counterparts. In particular, make sure any breaking changes

v4.3.0 vs v4.3.1 looks pretty minor, but includes the change we need. flask_appbuilder/security was not touched.


@potiuk
Copy link
Member

potiuk commented May 11, 2023

Did you check if there are no changes in security classes folowing this comment?

    # We are tightly coupled with FAB version because we vendored in part of FAB code related to security manager
    # This is done as part of preparation to removing FAB as dependency, but we are not ready for it yet
    # Every time we update FAB version here, please make sure that you review the classes and models in
    # `airflow/www/fab_security` with their upstream counterparts. In particular, make sure any breaking changes,

@potiuk
Copy link
Member

potiuk commented May 11, 2023

You can see example changes that needed to be ported in past PRs that were upgrading FAB BTW.

@VVildVVolf
Copy link
Contributor Author

@potiuk ,

You can see example changes that needed to be ported in past PRs that were upgrading FAB BTW.

Yes, I am already using https://github.com/apache/airflow/pull/29766/files as a reference, thanks!

Did you check if there are no changes in security classes folowing this comment?

Mostly I was focused on checking if upgrade of FAB does help or not with the issue #31156 . Good news that upgrading to 4.3.1 helps (but hard to be covered by Unit test, investigating if it is possible). The review of airflow/www/fab_security is in progress - I need to find out some details.

P.S. Just for history maybe it will help someone: to update FAB locally helped:

@potiuk
Copy link
Member

potiuk commented May 12, 2023

Mostly I was focused on checking if upgrade of FAB does help or not with the issue #31156 . Good news that upgrading to 4.3.1 helps (but hard to be covered by Unit test, investigating if it is possible). The review of airflow/www/fab_security is in progress - I need to find out some details.

From what I understand (I have not done it originally) the security manager was ported "partially" (that is what makes it difficult) - some parts of security manager is completely not used by us (and we did not port it), but some other (authentication types) are - and only changes to those should be ported.

@VVildVVolf
Copy link
Contributor Author

It looks like the difference v4.3.0 vs v4.3.1 looks pretty minor, but includes the change we need. flask_appbuilder/security was not touched.

@VVildVVolf VVildVVolf marked this pull request as ready for review May 12, 2023 20:17
@potiuk potiuk merged commit 1133035 into apache:main May 13, 2023
@potiuk
Copy link
Member

potiuk commented May 13, 2023

Thanks!

@potiuk potiuk added this to the Airflow 2.6.2 milestone May 13, 2023
@potiuk potiuk linked an issue May 15, 2023 that may be closed by this pull request
2 tasks
@eladkal eladkal added type:improvement Changelog: Improvements type:misc/internal Changelog: Misc changes that should appear in change log and removed type:improvement Changelog: Improvements labels Jun 8, 2023
eladkal pushed a commit that referenced this pull request Jun 8, 2023
(cherry picked from commit 1133035)
eladkal pushed a commit that referenced this pull request Jun 9, 2023
(cherry picked from commit 1133035)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Searching task instances by state doesn't work
3 participants