alan-turing-institute / data-safe-haven

https://data-safe-haven.readthedocs.io
BSD 3-Clause "New" or "Revised" License
61 stars 15 forks source link

Catch config upload validation errors #2211

Closed craddm closed 1 month ago

craddm commented 1 month ago

:white_check_mark: Checklist

:vertical_traffic_light: Depends on

:arrow_heading_up: Summary

Catches an exception when trying to upload an invalid config file and exits cleanly. Also improves the related error message to make it clearer what the validation error is.

Original error message: image

Catches the error cleanly and yields a clearer error message image

:closed_umbrella: Related issues

Closes #2210

:microscope: Tests

Tested locally

github-actions[bot] commented 1 month ago

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  data_safe_haven/commands
  config.py
Project Total  

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

JimMadge commented 1 month ago

Looks like a test is trying to use az CLI. Maybe a missing mock fixture.

craddm commented 1 month ago

Looks like a test is trying to use az CLI. Maybe a missing mock fixture.

Yes - odd as it was working before we changed things today, and the only thing we changed which touches this test is how the yaml with the missing field is made.

JimMadge commented 1 month ago

Might be due to a change on develop that was merged in?

craddm commented 1 month ago

Might be due to a change on develop that was merged in?

Nope - just tested it out. It works fine if I revert the change in how sre_config_yaml_missing_field is made. Can't understand why!

JimMadge commented 1 month ago

Hmm 🤔 something about how the sre_config_yaml fixture is constructed?

JimMadge commented 1 month ago

Or, the change from missing value to missing field has changed what is happening underneath?

JimMadge commented 1 month ago

In any case, you should be able to follow the traceback (or code) to find out what is happening.

craddm commented 1 month ago

We just weren't doing it correctly, so the yaml wasn't actually invalid and the upload function was progressing to the point where it needed to be logged in. The validation errors happen before that point, so it isn't necessary to do extra mocks to cope with logins etc.

JimMadge commented 1 month ago

Oh, that is brilliant. We were too good at not writing bad YAML 😆.