Add comments/docstrings to exceptions

[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).
• You have marked this pull request as a draft and added '[WIP]' to the title if needed (if you're not yet ready to merge).

⤴️ Summary

Adds descriptive comments and/or docstrings to exceptions.

Also checks consistency of usage across the codebase.

🌂 Related issues

Closes #1518

🔬 Tests

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
data_safe_haven/administration/users
entra_users.py					58-61
data_safe_haven/commands
shm.py					80-81, 94, 106, 114-115
sre.py					36, 108-111, 127, 148, 156-159
users.py					54-55, 90-91, 158-161, 199-200, 268-271
data_safe_haven/context
context_settings.py
data_safe_haven/exceptions
__init__.py
data_safe_haven/external/api
azure_api.py					866
graph_api.py					288, 772-781, 799-808, 826-835, 853-863
data_safe_haven/external/interface
azure_authenticator.py					71
azure_postgresql_database.py					229
data_safe_haven/functions
strings.py
data_safe_haven/infrastructure/components/dynamic
dsh_resource_provider.py					63, 70, 81
data_safe_haven/serialisers
yaml_serialisable_model.py
Project Total

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

[craddm]

Currently the custom exceptions use the following hierarchy:

[craddm]

I'm still trying to understand what we want the errors to achieve. At the moment they're a bit of a mishmash.

For example, DataSafeHavenParameterError is used when a user supplies a key for a context that either does not exist or already exists (when trying to create), but also when a yaml config file cannot be validated.

On the other hand, we have DataSafeHavenConfigError, which is used when no context file exists, or when a user supplies an SRE name that does not exist in the config, but also when no context is selected, or when the config file is not correct yaml.

Then we have DataSafeHavenInputError which is used when a subscription cannot be found, when there are missing values for users, when a secret already exists in a key-vault...

Finding it difficult to nail a description for some of these error classes as it's not always clear what they are intended to capture

[jemrobinson]

It's totally fine for you to redesign the error names and/or inheritance structure to make more sense. I think we want things to adhere to the following guidelines:

• everything should inherit from DataSafeHavenError eventually
• if X and Y both inherit from A it should be because they're conceptually both a type of A but that it's worth distinguishing between them (e.g. it might be nice to distinguish Azure errors from Graph errors even though they're both some kind of Microsoft/Cloud error).
• we (ideally) should be using each type of error in more than one place and, if possible, more than one file

Anything else you'd add to this @JimMadge ?

[jemrobinson]

@craddm is this still being worked on?

[craddm]

Yes - sorry, it's been bumped down the priority list a few times. I'll get back to it shortly.

[jemrobinson]

No problem - just checking that you weren't waiting for review.

[jemrobinson]

Um, what happened here @craddm ?

[craddm]

I tried to update it inline with develop, and apparently it went horribly wrong

[craddm]

Yes, I've decided I'm happy leaving DSHUserHandlingErrors alone for the time being, and happy to discuss whether to drop DSHType/ValueErrors here

[JimMadge]

Looks good.

I'm happy to keep DataSafeHaven{Value,Type}Error. Do we catch them anywhere?
If we don't I can see that we might want to in the future.

Looking at commands files, I think there are places where we need to replace raise DataSafeHaven* with

logger.critical(message)
Typer.Exit(1)

@craddm could you open an issue for that unless you want to fix it here?

[JimMadge]

@craddm Are you looking to fix all of the exceptions in commands here?

[craddm]

Possibly - am I on the right track?

[JimMadge]

Possibly - am I on the right track?

Yes, you just need to do something similar for every @typer.command().

[craddm]

I think I've got all the relevant ones now. There are some I've left alone as I think we don't want to exit at the point they occur, but could be wrong.

[craddm]

One thing I was unsure about was when I needed to decode response.content to print it as part of the error messages, done in 5598759

[JimMadge]

@craddm Are there any more raises in data_safe_haven/commands/*.py functions? I think all of those should be replaces with a message and Typer.Exit to gracefully end the program.

[craddm]

@craddm Are there any more raises in data_safe_haven/commands/*.py functions? I think all of those should be replaces with a message and Typer.Exit to gracefully end the program.

There are some nested try blocks in shm.py:

https://github.com/craddm/data-safe-haven/blob/43a9698716d7e77a6badd32b6c834a02ce1e011c/data_safe_haven/commands/shm.py#L91-L119

I interpret this as the inner exception being caught by the outer exception, and thus there's no need to typer.Exit the inner exception. Or perhaps the inner exception is redundant?

[JimMadge]

@craddm Yes those are OK because we catch them can raise typer.Exit().
There were a couple of others that would terminate the program ungracefully though.
I think 8caebec should fix that.

[JimMadge]

@craddm Ready to merge?