Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add more informative error messages to context commands #1916

Merged
merged 35 commits into from
Jun 6, 2024
Merged

Add more informative error messages to context commands #1916

merged 35 commits into from
Jun 6, 2024

Conversation

craddm
Copy link
Contributor

@craddm craddm commented May 23, 2024

✅ 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

N/A

⤴️ Summary

Adds more informative messages when users try to use context functions without first having created a context file.

Catches that user is not logged in when trying to create or teardown context infrastructure.

WIP: Catch that no context infrastructure is deployed

🌂 Related issues

Closes #1897
Closes #1899

🔬 Tests

tested locally

@JimMadge
Copy link
Member

@craddm Is this ready for review?

@craddm
Copy link
Contributor Author

craddm commented May 28, 2024

Pretty much - some tests failing as the commands now error when not logged in with Azure CLI. Do we know how to handle that?

@craddm craddm changed the title [WIP] Add more informative error messages to context commands Add more informative error messages to context commands May 28, 2024
@craddm craddm marked this pull request as ready for review May 28, 2024 13:44
@craddm craddm requested a review from a team as a code owner May 28, 2024 13:44
JimMadge
JimMadge previously requested changes May 29, 2024
View reviewed changes
data_safe_haven/commands/context.py Outdated Show resolved Hide resolved
data_safe_haven/commands/context.py Outdated Show resolved Hide resolved
data_safe_haven/context_infrastructure/infrastructure.py Outdated Show resolved Hide resolved
data_safe_haven/external/api/azure_api.py Outdated Show resolved Hide resolved
@JimMadge
Copy link
Member

@craddm Shall I pick up this one?

@github-actions GitHub Actions
Copy link

github-actions bot commented May 31, 2024 •
edited

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)		Lines missing
  data_safe_haven/commands
  context.py
  data_safe_haven/context_infrastructure
  infrastructure.py
  data_safe_haven/exceptions
  __init__.py
  data_safe_haven/external/api
  azure_api.py 143, 183, 262, 299, 333, 417, 475, 520, 541, 581, 625, 653, 690, 728, 772, 829, 845-849, 925, 953, 1006, 1038
  data_safe_haven/external/interface
  azure_authenticator.py 67-68
Project Total  

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

@JimMadge JimMadge requested review from JimMadge, jemrobinson and a team May 31, 2024 13:03
jemrobinson
jemrobinson reviewed May 31, 2024
View reviewed changes
Copy link
Member

@jemrobinson jemrobinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good. A couple of questions but nothing serious.

data_safe_haven/commands/context.py Outdated Show resolved Hide resolved
data_safe_haven/commands/context.py Outdated Show resolved Hide resolved
data_safe_haven/exceptions/__init__.py Show resolved Hide resolved
@jemrobinson
Copy link
Member

@JimMadge : are you still responsible for this PR? Happy to take it over if you're busy.

craddm and others added 10 commits June 4, 2024 15:27
@craddm @jemrobinson
Update data_safe_haven/commands/context.py
fc1161b 
Co-authored-by: James Robinson <james.em.robinson@gmail.com>
@craddm @jemrobinson
Update data_safe_haven/commands/context.py
23c9e71 
Co-authored-by: James Robinson <james.em.robinson@gmail.com>
@craddm @jemrobinson
Update data_safe_haven/commands/context.py
fb3050a 
Co-authored-by: James Robinson <james.em.robinson@gmail.com>
@craddm @jemrobinson
Update data_safe_haven/commands/context.py
fc1e649 
Co-authored-by: James Robinson <james.em.robinson@gmail.com>
@craddm @jemrobinson
Update data_safe_haven/commands/context.py
82cd6f8 
Co-authored-by: James Robinson <james.em.robinson@gmail.com>
@craddm @jemrobinson
Update data_safe_haven/commands/context.py
86beb56 
Co-authored-by: James Robinson <james.em.robinson@gmail.com>
@craddm @jemrobinson
Update data_safe_haven/commands/context.py
1be93bb 
Co-authored-by: James Robinson <james.em.robinson@gmail.com>
@craddm @jemrobinson
Update data_safe_haven/commands/context.py
74f6f60 
Co-authored-by: James Robinson <james.em.robinson@gmail.com>
@craddm @jemrobinson
Update data_safe_haven/commands/context.py
9da2cff 
Co-authored-by: James Robinson <james.em.robinson@gmail.com>
@jemrobinson
🔊 Use LoggingSingleton for error message before exit
89ab2c4
jemrobinson
jemrobinson approved these changes Jun 6, 2024
View reviewed changes
Copy link
Member

@jemrobinson jemrobinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jemrobinson jemrobinson merged commit 15195ff into alan-turing-institute:develop Jun 6, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jemrobinson jemrobinson jemrobinson approved these changes

@JimMadge JimMadge Awaiting requested review from JimMadge

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@craddm @JimMadge @jemrobinson