CyberNativeAI opened a new pull request, #74711: URL: https://github.com/apache/airflow/pull/74711
`airflow roles import` writes each entry as it goes, and every role and permission is committed on its own. When an entry names a resource or action that the target database does not have (a plugin view, a newer or older provider, a hand-edited file), the command stops at that entry with exit code 1. The roles and permissions written before it stay in the database. The re-run after the user fixes the file makes this worse. `roles_import` skips every role name that already exists, so the half-imported role is skipped. The command prints "roles and permissions successfully imported" and exits 0, and that role is still missing permissions. Repro on main (fresh SQLite DB, `db migrate` and `sync-perm`, then the import). The file gives `TeamViewer` access to DAG Runs, a plugin view `Data Quality Dashboard` and Task Instances, and gives `TeamOps` access to Pools: ``` $ airflow roles import unknown-resource.json Added can read on DAG Runs to role TeamViewer Added menu access on DAG Runs to role TeamViewer Added 2 permission(s) Resource named 'Data Quality Dashboard' does not exist rc=1 role TeamViewer: DAG Runs.can_read, DAG Runs.menu_access, Website.can_read role TeamOps: absent # same file without the plugin-view entry $ airflow roles import fixed.json Added can edit on Pools to role TeamOps Added can read on Pools to role TeamOps Added 2 permission(s) roles and permissions successfully imported rc=0 role TeamViewer: DAG Runs.can_read, DAG Runs.menu_access, Website.can_read <- Task Instances never added role TeamOps: Pools.can_edit, Pools.can_read, Website.can_read ``` An unknown action (`"action": "can_read,can_rerun"`) gives the same result. ### Change Before the first write, `roles_import` now checks the resource and action names of every entry it is going to import against the database. These are the same `get_resource` / `get_action` lookups that the add-permissions step already does for each entry. If any name is unknown, the command prints each unknown name once, prints "No roles were imported" and exits 1 before it writes any role or permission from the file. If all names are known, the import runs as before. With the change, the same two runs: ``` $ airflow roles import unknown-resource.json Resource named 'Data Quality Dashboard' does not exist No roles were imported rc=1 role TeamViewer: absent role TeamOps: absent $ airflow roles import fixed.json ... roles and permissions successfully imported rc=0 role TeamViewer: DAG Runs.can_read, DAG Runs.menu_access, Task Instances.can_read, Task Instances.menu_access, Website.can_read role TeamOps: Pools.can_edit, Pools.can_read, Website.can_read ``` What does not change: - An import that succeeds today gives the same result and output. The check covers only the entries that are imported, which are the entries the old code checked one at a time. Importing a valid file printed the same output and gave the same roles before and after the change. - Entries for roles that already exist are still skipped and are not checked, so a file that imports today does not start to fail. - `add-perms` and `del-perms` are not changed. This follows the approach that `[fab] custom_roles` (#72820) takes for the same reason: validate everything before creating any role, so that a failure does not leave a role that a later run skips. ### Testing - New parametrized test `test_cli_import_roles_with_unknown_names_changes_nothing`: unknown resource, unknown action, and several unknown names repeated across entries. Each case asserts exit code 1, each unknown name printed exactly once, and no role created. All three fail on main and pass with the change. A new test `test_cli_import_roles_skips_entries_of_existing_roles_before_checking_names` covers the unchanged skip rule. It passes on main and with the change. - `pytest providers/fab/tests/unit/fab/auth_manager/cli_commands/test_role_command.py` on SQLite with Python 3.11: 13 passed with the change, and 9 passed for main's version of the file on main. - On a newly created test DB, the existing tests that use `Pools` exit with "Resource named 'Pools' does not exist" (main and this branch alike), because the FAB resources do not exist until permissions are synced. The results above used a DB where `airflow sync-perm` had run after the test DB init. - `prek run --files` on both files: passed. `mypy` on `role_command.py`: no issues. I could not run the manual `mypy-providers` hook because breeze is not installed, and I did not test on PostgreSQL or MySQL. No changelog entry, because the change is not breaking. related: #72796, #72820 --- ##### Was generative AI tooling used to co-author this PR? - [X] Yes (please specify the tool below) Generated-by: Claude Code (Opus 5.5) following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions) --- * Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged. * For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed. * When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x). * For significant user-facing changes create newsfragment: `{pr_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments). You can add this file in a follow-up commit after the PR is created so you know the PR number. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
