Cli export / import roles including permissions

[dominikhei]

closes: #36128

This PR contains both an updated airflow roles export and airflow roles import cli command, according to the above mentioned issue.

The export command now exports resources and actions by default in the following structure:

[{"name": "FakeTeamA", "resource": "Pools", "action": "can_edit,can_read"}, 
 {"name": "FakeTeamA", "resource": "Admin", "action": ",menu_access"}, 
{"name": "FakeTeamB", "resource": "", "action": ""}]

• When a role has no permissions the values for the respective keys are set to empty Strings.
  This way the import function can later easily distinguish if the imported role needs permissions to be set or not

• @potiuk I originally mentioned that we could use the -p flag to say whether we want to export names and permissions or just the names. However the function already uses the -p flag (--pretty) thus I believe this will not work. That's why the function currently also exports permissions by default.

• Previously the -p flag sorted the list of names in alphabetical order and set the indent to 4 . However with the function now returning a list of dictionaries, I disabled the sorting if -pis set, because in this case the order of the keys would be switched.

The airflow roles import commands expect the json to be in the same structure as the exported one. In it I have reused the __roles_add_or_remove_permissionswhich was already contained in the file and handles the logic for adding permissions.

• to pass args to __roles_add_or_remove_permissions from the imported json, I used Namespace from argparse. argparse is already a dependency of airflow.

• In the beginning, I remove all roles that are already contained within the db from the json. By this I can ensure that only new roles are being added. Due to the structure of the json, role names are contained multiple time with different resources, thus I need to check for each dictionary, whether the role has been already added and thus I only need to add the permissions within this dictionary or a new role needs to be created.

Unit tests have been added to test the new functions.

---

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

[potiuk]

LGTM as an approach. My proposal will be to replace the whole Role if it is already defined with the new one coming from the. import. I wonder if we should backwards compatibility and add --include-permissions - but I think we do not need to - the export/import was indeed pretty useless without the permissions so I'd treat that as a bugfix, but I wonder what others think about it ?

[potiuk]

LGTM but I would look for other's opinions?

[gbonazzoli]

@dominikhei

I've just tested on my installation and it works !

There is only a little aesthetic inaccuracy as you can see form the airflow roles export output:

airflow@airflow-kube-worker-848886d894-gv4wn:~$ airflow roles export b.json
38 roles with permissions successfully exported to b.json

it says 38 roles, actually they are only 4 roles but the permissions associated have multiplied them.

I looked at the code and I think we have the same problem also in the import phase.

[potiuk]

I looked at the code and I think we have the same problem also in the import phase.

Good chance to make a PR and fix it @gbonazzoli

[gbonazzoli]

@potiuk @dominikhei

Unfortunately I'm not a developer and I'm not able to make the PR (git commands and count distinct "name" attribute in the Python list are among the things that I do not know how to write).

Anyway, If I was a developer I should print out: 4 roles exported with 38 linked/granted permissions.

linked in export and granted in import

[potiuk]

Unfortunately I'm not a developer and I'm not able to make the PR (git commands and count distinct "name" attribute in the Python list are among the things that I do not know how to write).

Sounds like a cool - REALLY SMALL - issue to start to learn being developer. It's not as hard. And that one looks like super easy fix.

[potiuk]

And BTW, if you don't want to - then I suggest you create an issue - then we can mark it as good-first-issue and make it ready for someone to pick it up. Comment in PR is not really actionable and will just gone forgotten if there is no issue.

[vincbeck]

By looking at this PR again, the json format changes, is not it a breaking change? If some users use the CI command airflow roles export or airflow roles import in their CI, it will fail. Also, this PR seems introducing a flaky test. If you run breeze testing db-tests --parallel-test-types "Always Providers[fab]", it will fail most of the time. I dont know how the CI passed (maybe just luck?)

[vincbeck]

To fix the test: #36558

[potiuk]

By looking at this PR again, the json format changes, is not it a breaking change?

The observation we had is that we should treat it as bugfix. Roles export/import without permissions is kinda useless, so it's rather unlikely anyone would use it.

[vincbeck]

Sounds good :)

[gbonazzoli]

@potiuk @vincbeck @dominikhei

#36580 created with the title: Cli export / import roles including permissions Part 2 (refinement)