From 33234be4af9f2d690d019ce0864382a0a45cafb4 Mon Sep 17 00:00:00 2001 From: Ed Morley <501702+edmorley@users.noreply.github.com> Date: Mon, 20 Jun 2022 16:48:10 +0100 Subject: [PATCH] Remove `ProcfileParsingError::RegexError` as a user-facing error (#77) There is currently only one error variant for `ProcfileParsingError`, an error that only occurs if it was not possible to compile the regex used to parse a `Procfile`. This regex is buildpack-supplied, so if such an error occurs, it's always an internal buildpack error. However previously a user-facing error message was shown, that told the user to check their `Procfile` was valid, which has no bearing on whether the regex will compile. Rather than just correct the error message, the error has been removed and instead `.expect()` used, since such regex errors will not happen in practice, given they will be caught by both the `invalid_regex` Clippy lint and the buildpack's tests. I believe part of the reason for this confusion over error messages is that the error message handling was split across multiple files due to the use of `thiserror` meaning that the display representation could be leant upon, without actually looking closely at what was being displayed. As such, I've removed `thiserror`, since I think the error handling is easier to reason about without it. Fixes #74. GUS-W-11318868. --- CHANGELOG.md | 1 + Cargo.lock | 1 - Cargo.toml | 1 - src/error.rs | 66 +++++++++++++++++++------------------------------ src/launch.rs | 3 +-- src/procfile.rs | 19 +++++++------- 6 files changed, 36 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 12216ba..5464a18 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## Unreleased +- Remove incorrect error message shown in the case of internal buildpack regex errors ([#77](https://github.com/heroku/procfile-cnb/pull/77)). - Updated `libcnb` and `libherokubuildpack` from 0.5.0 to 0.7.0 ([#49](https://github.com/heroku/procfile-cnb/pull/49) and [#60](https://github.com/heroku/procfile-cnb/pull/60)). ## 1.0.1 diff --git a/Cargo.lock b/Cargo.lock index 23642c2..094ea7f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -787,7 +787,6 @@ dependencies = [ "libherokubuildpack", "linked-hash-map", "regex", - "thiserror", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index e94bc41..32e9009 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,7 +12,6 @@ libcnb = "0.7.0" libherokubuildpack = "0.7.0" linked-hash-map = "0.5.4" regex = "1.5.6" -thiserror = "1.0.31" [dev-dependencies] libcnb-test = "0.3.1" diff --git a/src/error.rs b/src/error.rs index 0b3c3df..fffba1f 100644 --- a/src/error.rs +++ b/src/error.rs @@ -2,14 +2,11 @@ use crate::launch::ProcfileConversionError; use crate::procfile::ProcfileParsingError; use indoc::formatdoc; -#[derive(thiserror::Error, Debug)] +#[derive(Debug)] pub enum ProcfileBuildpackError { - #[error("Cannot read Procfile contents: {0}")] CannotReadProcfileContents(std::io::Error), - #[error("Procfile parsing error: {0}")] - ProcfileParsingError(#[from] ProcfileParsingError), - #[error("Procfile conversion error: {0}")] - ProcfileConversionError(#[from] ProcfileConversionError), + ProcfileParsingError(ProcfileParsingError), + ProcfileConversionError(ProcfileConversionError), } pub fn error_handler(buildpack_error: ProcfileBuildpackError) -> i32 { @@ -22,43 +19,30 @@ pub fn error_handler(buildpack_error: ProcfileBuildpackError) -> i32 { encoded file and try again. Underlying cause was: {io_error} - ", - io_error = io_error - }, - ); - } - ProcfileBuildpackError::ProcfileParsingError(parsing_error) => { - libherokubuildpack::log_error( - "Cannot parse Procfile", - formatdoc! {" - Please ensure the Procfile in the root of your application is a readable UTF-8 - encoded Procfile as specified on Heroku DevCenter and try again: - - https://devcenter.heroku.com/articles/procfile - - Underlying cause was: {parsing_error} - ", - parsing_error = parsing_error - }, - ); - } - ProcfileBuildpackError::ProcfileConversionError(conversion_error) => { - libherokubuildpack::log_error( - "Cannot convert Procfile to CNB launch configuration", - formatdoc! {" - This is an unexpected internal error that occurs when a Procfile entry is not - compatible with the CNB launch configuration. At the time of writing, Procfile - process names are a strict subset of CNB launch process names and this should - never happen. - - Please report this issue with the details below. - - Details: {conversion_error} - ", - conversion_error = conversion_error - }, + "}, ); } + // There are currently no ways in which parsing can fail, however we will add some in the future: + // https://github.com/heroku/procfile-cnb/issues/73 + ProcfileBuildpackError::ProcfileParsingError(parsing_error) => match parsing_error {}, + ProcfileBuildpackError::ProcfileConversionError(conversion_error) => match conversion_error + { + ProcfileConversionError::InvalidProcessType(libcnb_error) => { + libherokubuildpack::log_error( + "Cannot convert Procfile to CNB launch configuration", + formatdoc! {" + This is an unexpected internal error that occurs when a Procfile entry is not + compatible with the CNB launch configuration. At the time of writing, Procfile + process names are a strict subset of CNB launch process names and this should + never happen. + + Please report this issue with the details below. + + Details: {libcnb_error} + "}, + ); + } + }, } 1 diff --git a/src/launch.rs b/src/launch.rs index cf87ced..0d3d9d3 100644 --- a/src/launch.rs +++ b/src/launch.rs @@ -29,9 +29,8 @@ impl TryFrom for Launch { } } -#[derive(thiserror::Error, Debug)] +#[derive(Debug)] pub enum ProcfileConversionError { - #[error("Incompatible process type")] InvalidProcessType(libcnb::data::launch::ProcessTypeError), } diff --git a/src/procfile.rs b/src/procfile.rs index 585a9b5..57acf8e 100644 --- a/src/procfile.rs +++ b/src/procfile.rs @@ -33,14 +33,14 @@ impl FromStr for Procfile { type Err = ProcfileParsingError; fn from_str(procfile_contents: &str) -> Result { - let re_carriage_return_newline = - Regex::new("\\r\\n?").map_err(ProcfileParsingError::RegexError)?; - let re_multiple_newline = - Regex::new("\\n*\\z").map_err(ProcfileParsingError::RegexError)?; + // Using `.expect()` since these can only fail if we've supplied invalid an invalid regex, + // which would be caught by both the `invalid_regex` Clippy lint and the buildpack's tests. + let re_carriage_return_newline = Regex::new("\\r\\n?").expect("Invalid Procfile regex"); + let re_multiple_newline = Regex::new("\\n*\\z").expect("Invalid Procfile regex"); // https://github.com/heroku/codon/blob/2613554383cb298076b4a722f4a1aa982ad757e6/lib/slug_compiler/slug.rb#L538-L545 let re_procfile_entry = Regex::new("^[[:space:]]*([a-zA-Z0-9_-]+):?\\s+(.*)[[:space:]]*") - .map_err(ProcfileParsingError::RegexError)?; + .expect("Invalid Procfile regex"); let procfile_contents = re_carriage_return_newline.replace_all(procfile_contents, "\n"); let procfile_contents = re_multiple_newline.replace(&procfile_contents, "\n"); @@ -62,11 +62,10 @@ impl FromStr for Procfile { } } -#[derive(thiserror::Error, Debug, PartialEq)] -pub enum ProcfileParsingError { - #[error("Regex error: {0}")] - RegexError(#[from] regex::Error), -} +// There are currently no ways in which parsing can fail, however we will add some in the future: +// https://github.com/heroku/procfile-cnb/issues/73 +#[derive(Debug, PartialEq)] +pub enum ProcfileParsingError {} #[cfg(test)] mod tests {