You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/01/14 12:39:59 UTC
[GitHub] [airflow] ephraimbuddy opened a new pull request #20874: Add downgrade to some FAB migrations
ephraimbuddy opened a new pull request #20874:
URL: https://github.com/apache/airflow/pull/20874
There are some FAB migrations that don't have downgrades.
This PR fixes it
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #20874: Add downgrade to some FAB migrations
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20874:
URL: https://github.com/apache/airflow/pull/20874#discussion_r785639228
##########
File path: airflow/migrations/versions/849da589634d_prefix_dag_permissions.py
##########
@@ -167,4 +233,8 @@ def upgrade():
def downgrade():
- pass
+ db = SQLA()
+ db.session = settings.Session
+ undo_migrate_to_new_dag_permissions(db)
+ db.session.commit()
Review comment:
This commit seems unnecessary?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy merged pull request #20874: Add downgrade to some FAB migrations
Posted by GitBox <gi...@apache.org>.
ephraimbuddy merged pull request #20874:
URL: https://github.com/apache/airflow/pull/20874
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #20874: Add downgrade to some FAB migrations
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20874:
URL: https://github.com/apache/airflow/pull/20874#issuecomment-1013191560
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy closed pull request #20874: Add downgrade to some FAB migrations
Posted by GitBox <gi...@apache.org>.
ephraimbuddy closed pull request #20874:
URL: https://github.com/apache/airflow/pull/20874
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #20874: Add downgrade to some FAB migrations
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20874:
URL: https://github.com/apache/airflow/pull/20874#discussion_r785639839
##########
File path: airflow/migrations/versions/849da589634d_prefix_dag_permissions.py
##########
@@ -167,4 +233,8 @@ def upgrade():
def downgrade():
- pass
+ db = SQLA()
+ db.session = settings.Session
+ undo_migrate_to_new_dag_permissions(db)
+ db.session.commit()
Review comment:
Aos from what I can tell `undo_migrate_to_new_dag_permissions` only uses `db.session`; why not just pass that into the function, but the entire `db` object?
##########
File path: airflow/migrations/versions/849da589634d_prefix_dag_permissions.py
##########
@@ -167,4 +233,8 @@ def upgrade():
def downgrade():
- pass
+ db = SQLA()
+ db.session = settings.Session
+ undo_migrate_to_new_dag_permissions(db)
+ db.session.commit()
Review comment:
Also from what I can tell `undo_migrate_to_new_dag_permissions` only uses `db.session`; why not just pass that into the function, but the entire `db` object?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #20874: Add downgrade to some FAB migrations
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20874:
URL: https://github.com/apache/airflow/pull/20874#discussion_r785639167
##########
File path: airflow/migrations/versions/2c6edca13270_resource_based_permissions.py
##########
@@ -319,3 +344,7 @@ def upgrade():
def downgrade():
"""Unapply Resource based permissions."""
+ log = logging.getLogger()
+ handlers = log.handlers[:]
Review comment:
Worth adding a note why this is needed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #20874: Add downgrade to some FAB migrations
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20874:
URL: https://github.com/apache/airflow/pull/20874#discussion_r785757340
##########
File path: airflow/migrations/versions/2c6edca13270_resource_based_permissions.py
##########
@@ -319,3 +344,7 @@ def upgrade():
def downgrade():
"""Unapply Resource based permissions."""
+ log = logging.getLogger()
+ handlers = log.handlers[:]
Review comment:
I think this logging is not necessary, however, I copied it from the upgrade function
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #20874: Add downgrade to some FAB migrations
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #20874:
URL: https://github.com/apache/airflow/pull/20874#discussion_r784945901
##########
File path: airflow/migrations/versions/849da589634d_prefix_dag_permissions.py
##########
@@ -55,6 +55,22 @@ def prefix_individual_dag_permissions(session):
session.commit()
+def remove_prefix_in_individual_dag_permissions(session):
+ dag_perms = ['can_dag_read', 'can_dag_edit']
Review comment:
```suggestion
dag_perms = ['can_read', 'can_edit']
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org