Repository navigation
Add more context to LXD system check error messages - #1126
jonathan-conder wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new LXD error-handling branches lack regression tests for diagnostic context and preservation of the original error.
1 open issue
What changed in this PR
This PR improves Workshop’s LXD system-check diagnostics, addressing #1121 by identifying LXD and the failed operation while preserving the underlying error.
Changes:
- Distinguishes LXD API status errors from generic connection failures.
- Adds operation-specific context to storage, network, version, and workshop-listing errors.
| File | Description |
|---|---|
internal/workshop/lxd/lxd_backend.go |
Wraps system-check and backend-setup errors with explicit LXD context. |
Review effort: Balanced
tlm
left a comment
tlm
left a comment
There was a problem hiding this comment.
Some initial feedback for this PR. I have read the issue this is trying to fix.
The work needs some more design applied to and overall thinking. Can I suggest thinking about this some more and applying the following principles.
- Deep down the call stack in a backend is not the place to be attempting to write UX based errors for consumption by a user. We can never achieve good UX this way so we shouldn't try.
- It is only at our UX interface boundaries that we should be attempting to produce good clean helpful error messages for users.
- To achieve this we identify the failure cases that can reasonably be communicated and used typed errors up the stack to communicate this fact.
- Funcs that produce error messages should include context about the error where the context was not supplied by the caller.
- Callers should add further context to the error as it goes up the stack.
- "Cannot" is not how errors should get started. This is a given pre-condition hence the error. Start errors with a verb of what was trying to be achieved.
- Don't add extra words to errors to describe what a value was going to be used for later in a routine. The error is just to deal with the immediate failure and shouldn't imply intent until intent is actually being asserted.
My justification for this is that the problem we are attempting to fix is a UX error issue. Doing it this way is fragile, non testable and leads to weak contracts.
| Maybe LXD isn't installed? | ||
| To install LXD: 'sudo snap install --channel=6/stable lxd'`, err) | ||
| case api.StatusErrorCheck(err): | ||
| return fmt.Errorf("cannot set up LXD connection: %w", err) |
There was a problem hiding this comment.
cannot is a given hence the error. Errors should start with the verb that failed. In this case "connecting", "setting up", "setup" etc.
Can I also make the recommendation that you also refactor the name of the function to imply that it only deals with errors that occur during connection to a lxd backend?
All of the current use cases of it only do so in this context and it is written to only deal with this context.
The func should also not be exported on the package while we are here.
| } | ||
| if err != nil { | ||
| return err | ||
| return fmt.Errorf("cannot query LXD storage pool free space: %w", err) |
There was a problem hiding this comment.
| return fmt.Errorf("cannot query LXD storage pool free space: %w", err) | |
| return fmt.Errorf("getting storage pool %q resources to check free space: %w", storagePool, err) |
| info, _, err := conn.GetServer() | ||
| if err != nil { | ||
| return err | ||
| return fmt.Errorf("cannot query LXD version: %w", err) |
There was a problem hiding this comment.
| return fmt.Errorf("cannot query LXD version: %w", err) | |
| return fmt.Errorf("getting lxd server information: %w", err) |
We are asking for more then just version in this context. The version check happens after this. The error should match the intent not the usage that happens afterwards.
Description
Fixes #1121.
Self-review quick check
Docs
Procedure:
Content:
tutorial/andhow-to/sections).docs/.coverage.yamlupdated, coverage tags added (.. artefact).Or: