-
Notifications
You must be signed in to change notification settings - Fork 2
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.
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
Adds error handling routines and messages #47
Conversation
c24d9ec
to
702a4a9
Compare
* Error messages for review Stacked PR related to #47 * Added error context * Apply suggestions from code review Co-authored-by: Helen Cheng <[email protected]> * Update src/errors.rs * Update src/errors.rs * Update src/errors.rs * Fix line endings and punctuation * Apply suggestions from code review Co-authored-by: Helen Cheng <[email protected]> * Update src/errors.rs * Updated error messages --------- Co-authored-by: Helen Cheng <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm mid-way through. I started with the non error.rs files and I've looked through all of them.
I noticed there's one error not being used:
#[derive(Debug)]
#[allow(dead_code)] // TODO: remove this once error messages are added
pub(crate) struct UnsupportedArchitectureNameError(String);
let architecture =
ArchitectureName::from_str(&target_arch).map_err(|_| UnsupportedDistroError {
name: name.to_string(),
version: version.to_string(),
architecture: target_arch.to_string(),
})?;
Appart from that: I'm trying to: Gather context, look for logic errors or mistakes that might have been accidentally introduced. I'm also thinking about user experience in debugging and as a buildpack dev.
While doing that I'm investigating and mentioning things as I see them because I'm inconsistently working on reviews here, if I don't say stuff now I don't know when I'll get another chance.
I wanted to leave my in-progress review so you have something to poke at while I keep on looking on to the error.rs
where the bulk of the logic lives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finished the review of the errors. I love the output tests. It's a lot of extra lines, but I used $ cargo tarpaulin --out Html
to validate that all relevant output was exercised by tests and it's showing 99%+ line coverage:
I made a handful of comments.
Requested: I would like to see a softening of the language when suggesting people open issues. I understand that there's a lot of them here where we don't know the true underlying problem. There are some issues such as the SHA being wrong where you've indicated actually would really only be possible if there was a bug in the buildpack. I am okay elevating these errors and saying "really, you're doing us a favor opening an issue." But for the rest, I'm worried that an incident that affects disk IO in unexpected ways might prompt a flurry of issues that aren't the responsibility of the buildpack (for example).
@schneems I believe I've addressed most of your feedback that fits within the scope of these changes (apart from some of the style comments). Out of scope feedback has been moved into issues or delegated to a future discussion. |
Thanks for the fast turnaround and for incorporating some of the style feedback. I un-resolved this discussion and left a comment #47 (comment). This is my only blocker to merging this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks a ton for iterating on this
Short: My position of "never include open a support ticket or issue in an error message" has softened given these circumstances and desires. I wanted to document in text why I think that's okay to help evolve our understanding of error best practices. I still feel that in the long term it's not something we'll want to encourage a lot of, but given the states above if messages are unseen or actively being iterated on, then that captures a healthy and sustainable "start -> maturity -> stable maintenance" progression path. Long: I wanted to add, as part of this conversation https://github.com/heroku/buildpacks-deb-packages/pull/47/files/dc1ba691e88b9dafae82b18f998d446f89a73c70#r1792430802. Colin and I spoke in chat. We came to a consensus to frame that approach as an experiment
My interpretation of this is that: The errors with the "please open an issue" either want to never-be-seen, in which case there's we were right that it's a failure mode that doesn't need more investment, or have something changed (the code, the error message itself, etc.) until they either no longer need a "please open an issue" or that failure mode is no longer possible. Some of my concerns included: Forked buildpacks will still carry URLs pointing at this repo. I think that's true, but less likely for this specific buildpack (versus the Ruby or Python one). Also, since this is an entirely net-new codebase (dpkg logic was effectively re-written in rust) that it's good to solicit feedback for fast iteration. Versus the Ruby buildpack is highly used and a well-worn path and I trust any issues that exist will be raised. |
The bulk of this PR is the error messages found in errors.rs. All other changes are modifications to the existing error structs and enums to pass through relevant data such as file paths on I/O errors, etc.
One interesting thing to note is that the error message builder used here allows for clearly marking certain types of errors as:
Framework
- for libcnb framework errorsInternal
- for unexpected errors such as I/O errors and other issues where we the issue is unlikely to be the fault of the user but where a developer using this buildpack might benefit from a more detailed messageUserFacing
- for errors that are due to things like misconfigurations, networking issues, etc. which provide information + actions that a user can takeRelated PRs: