Raise exception when admin group name is not found

[craddm]

✅ Checklist

• You have given your pull request a meaningful title (e.g. Enable foobar integration rather than 515 foobar).
• You are targeting the appropriate branch. If you're not certain which one this is, it should be develop.
• Your branch is up-to-date with the target branch (it probably was when you started, but it may have changed since then).

🚦 Depends on

⤴️ Summary

Raises an exception when the admin group name is specified incorrectly in the context.

🌂 Related issues

Closes #2187

🔬 Tests

Tested locally

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
data_safe_haven/config
shm_config.py					31-37
data_safe_haven/external/api
azure_sdk.py					439
graph_api.py					143, 525-529, 1031
Project Total

This report was generated by python-coverage-comment-action

[JimMadge]

Returning None is important here where if there is no group then one is created.

[JimMadge]

Also used in remove_user_from_group and add_user_to_group where None is not handled.

A good solution could be adding a method like

def ensure_id_from_groupname(name):
   if (id := get_id_from_groupname(name)):
       return id
   else:
       raise Exception

[craddm]

Also used in remove_user_from_group and add_user_to_group where None is not handled.

A good solution could be adding a method like

def ensure_id_from_groupname(name):
   if (id := get_id_from_groupname(name)):
       return id
   else:
       raise Exception

This feels weird to me.

We have a function called get_id_from_groupname which gets the id if the group exists or returns None, and now a function called ensure_id_from_groupname which also gets the id if the group exists, but throws an exception if it doesn't exist. I feel like that is a bit confusing, especially given that (almost) all the other ensure_* functions check if something exists and create it if it doesn't.

It feels like we should have a function that checks if the group exists, and then one that returns the group id (but is only used when we know the group exists). The slight problem with that is the way to check if the group exists is to try to retrieve the whole list of groups and search through it, which you then would have to do multiple times instead of once.

[JimMadge]

This is the internal API so I wouldn't worry too much about names.

You could change how the current create_group works to return the name if it exists but that might be too much for one function.

Making two API calls when you only need one feels wasteful.
I still think just adding a method for the common pattern of ensuring the groups exists makes most sense. It is just taking the code that would be duplicated in a number of places and instead writing it once.

[craddm]

This is the internal API so I wouldn't worry too much about names.

You could change how the current create_group works to return the name if it exists but that might be too much for one function.

Making two API calls when you only need one feels wasteful. I still think just adding a method for the common pattern of ensuring the groups exists makes most sense. It is just taking the code that would be duplicated in a number of places and instead writing it once.

Agree about the multiple API calls, which would be easy enough to stop by instead passing the list of groups to the functions from a separate call to read_groups(). But anyway, gone back to your suggested style

[JimMadge]

LGTM 👍