Conversation
`airflow roles export` writes a role that holds no permissions as an entry with an empty resource and action, so the command cannot always read back its own output. Importing such a file aborted outright when that entry came first, and silently re-applied the preceding entry's permissions when it came later — both of which break the export/import round trip these two commands exist to serve. The existing coverage missed this because its permissionless entry sits last in the fixture, where the stale binding from an earlier iteration papers over the missing one.
rjgoyln
left a comment
There was a problem hiding this comment.
LGTM. Checked the mechanism rather than the symptom: FAB's Role defines no __eq__, so role_dict["name"] in appbuilder.sm.get_all_roles() compares a str against ORM objects and is always false — the outer branch always ran and the mirrored in block below it was dead — and both add_role implementations (upstream FAB and the provider's override.py) return the existing role, so calling it unconditionally is safe. Appending the new test to unpatched main does reproduce UnboundLocalError: cannot access local variable 'role_args', so it fails without the source change.
Two non-blocking notes below: one on the reordered key access in roles_import, one on an assertion in the new test.
Just my thoughts, feel free to resolve it.
Drafted-by: Claude Code (Opus 5); reviewed by @rjgoyln before posting
Co-authored-by: rjgoyln <151457491+rjgoyln@users.noreply.github.com>
Co-authored-by: rjgoyln <151457491+rjgoyln@users.noreply.github.com>
airflow roles exportwrites a role that holds no permissions as an entry with anempty
resourceandaction, soairflow roles importcannot always read backwhat
roles exportproduced.Importing such a file behaved differently depending on where that entry sat:
UnboundLocalError: cannot access local variable 'role_args', after the role had already been committed, leavinga half-finished import;
permissions, because
role_argswas still bound from an earlier iteration.Either way the export/import round trip that these two commands exist to serve
was broken.
roles_importnow adds the role unconditionally (add_rolereturns the existingrole, so a role spanning several resources — which
roles_exportemits as severalsame-named entries — is safe) and skips the permission step when the entry carries
no resource or action, so the
Namespaceis built only where it is used.Two surrounding branches went away with it: the outer
role_dict["name"] not in appbuilder.sm.get_all_roles()compares astragainsta list of
RoleORM objects and is therefore always true, which made the mirroredinblock below it unreachable.Why the existing test did not catch this
test_cli_import_rolesalready covers a permissionless role, but places it lastin the fixture, where the stale binding from an earlier iteration papers over the
missing one. The new test puts it first.
Testing
uv run --project providers/fab pytest providers/fab/tests/unit/fab/auth_manager/cli_commands/test_role_command.py— 9 passed.test_cli_import_roles_when_a_role_without_permissions_comes_firstwith the realUnboundLocalError, and nothing else.airflow roles importon[{"name": "X", "resource": "", "action": ""}]raised
UnboundLocalErroratrole_command.py:219.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 5) following the guidelines