Update printing style (#252)

* Add bullet_stream for printing

* Update expect messages

* Update main printing

* Update error printing

* Update integration tests

* Changelog

* Include filename in errors

By default Rust's `fs` module does not include the filename when an error is returned. The `fs_err` crate adds filenames back in.

* Add where processes are coming from

While the name of the buildpack "Procfile Buildpack" hints that it's looking at a Procfile, be specific with the value.

* Explain when no process are found

The output `(none)` is not a full sentence. This explains that the buildpack saw an empty file (not entirely true, it could be an error or mis-parse, but there's an open issue for that) and as a result defined no processes.

* Update changelog

Author: schneems

CHANGELOG.md

@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+
+- Build output style updated to `bullet_stream`. Output now also includes the commands that were pulled from the `Procfile` in the output. ([#252](https://github.com/heroku/buildpacks-procfile/pull/252))
+
 ## [3.1.2] - 2024-07-02
 
 ### Changed

Cargo.lock

@@ -14,6 +14,8 @@ pedantic = { level = "warn", priority = -1 }
 unwrap_used = "warn"
 
 [dependencies]
+bullet_stream = "0.3.0"
+fs-err = "3.0.0"
 indoc = "2"
 libcnb = { version = "0.26", features = ["trace"] }
 libherokubuildpack = { version = "0.26", default-features = false, features = ["error", "log"] }

Cargo.toml

@@ -1,7 +1,7 @@
 use crate::launch::ProcfileConversionError;
 use crate::procfile::ProcfileParsingError;
+use bullet_stream::Print;
 use indoc::formatdoc;
-use libherokubuildpack::log::log_error;
 
 #[derive(Debug)]
 pub(crate) enum ProcfileBuildpackError {
@@ -11,37 +11,36 @@ pub(crate) enum ProcfileBuildpackError {
 }
 
 pub(crate) fn error_handler(buildpack_error: ProcfileBuildpackError) {
+    let build_output = Print::new(std::io::stdout()).without_header();
     match buildpack_error {
         ProcfileBuildpackError::CannotReadProcfileContents(io_error) => {
-            log_error(
-                "Cannot read Procfile contents",
-                formatdoc! {"
-                    Please ensure the Procfile in the root of your application is a readable UTF-8
-                    encoded file and try again.
-
-                    Underlying cause was: {io_error}
-                "},
-            );
+            build_output.error(formatdoc! {"
+                Cannot read Procfile contents
+
+                Please ensure the Procfile in the root of your application is a readable UTF-8
+                encoded file and try again.
+
+                Underlying cause was: {io_error}
+            "});
         }
         // There are currently no ways in which parsing can fail, however we will add some in the future:
         // https://github.com/heroku/buildpacks-procfile/issues/73
         ProcfileBuildpackError::ProcfileParsingError(parsing_error) => match parsing_error {},
         ProcfileBuildpackError::ProcfileConversionError(conversion_error) => match conversion_error
         {
             ProcfileConversionError::InvalidProcessType(libcnb_error) => {
-                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}
-                    "},
-                );
+                build_output.error(formatdoc! {"
+                    Cannot convert Procfile to CNB launch configuration
+
+                    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}
+                "});
             }
         },
     }

src/error.rs

@@ -4,12 +4,12 @@ mod procfile;
 
 use crate::error::{error_handler, ProcfileBuildpackError};
 use crate::procfile::Procfile;
+use bullet_stream::{style, Print};
 use libcnb::build::{BuildContext, BuildResult, BuildResultBuilder};
 use libcnb::detect::{DetectContext, DetectResult, DetectResultBuilder};
 use libcnb::generic::{GenericMetadata, GenericPlatform};
 use libcnb::{buildpack_main, Buildpack};
-use libherokubuildpack::log::{log_header, log_info};
-use std::fs;
+use std::io::stdout;
 use std::path::Path;
 
 #[cfg(test)]
@@ -31,20 +31,26 @@ impl Buildpack for ProcfileBuildpack {
     }
 
     fn build(&self, context: BuildContext<Self>) -> libcnb::Result<BuildResult, Self::Error> {
-        log_header("Discovering process types");
+        let mut bullet = Print::new(stdout())
+            .h2("Procfile Buildpack")
+            .bullet(format!("Processes from {}", style::value("Procfile")));
 
-        let procfile = fs::read_to_string(context.app_dir.join("Procfile"))
+        let procfile: Procfile = fs_err::read_to_string(context.app_dir.join("Procfile"))
             .map_err(ProcfileBuildpackError::CannotReadProcfileContents)
             .and_then(|procfile_contents| {
                 procfile_contents
                     .parse()
                     .map_err(ProcfileBuildpackError::ProcfileParsingError)
             })?;
 
-        log_info(format!(
-            "Procfile declares types -> {}",
-            format_processes_for_log(&procfile)
-        ));
+        if procfile.is_empty() {
+            bullet = bullet.sub_bullet("Empty file, no processes defined");
+        } else {
+            for (name, command) in &procfile.processes {
+                bullet = bullet.sub_bullet(format!("{name}: {}", style::command(command)));
+            }
+        }
+        bullet.done().done();
 
         BuildResultBuilder::new()
             .launch(
@@ -64,19 +70,6 @@ fn dir_has_procfile(app_dir: impl AsRef<Path>) -> bool {
     app_dir.as_ref().join("Procfile").exists()
 }
 
-fn format_processes_for_log(procfile: &Procfile) -> String {
-    if procfile.is_empty() {
-        String::from("(none)")
-    } else {
-        procfile
-            .processes
-            .keys()
-            .cloned()
-            .collect::<Vec<String>>()
-            .join(", ")
-    }
-}
-
 // Implements the main function and wires up the framework for the given buildpack.
 buildpack_main!(ProcfileBuildpack);
 
@@ -96,27 +89,4 @@ mod tests {
         let app_dir = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/missing_procfile");
         assert!(!dir_has_procfile(app_dir));
     }
-
-    #[test]
-    fn test_empty_names_from_processes() {
-        let procfile = Procfile::new();
-
-        let out = format_processes_for_log(&procfile);
-        assert_eq!(out, "(none)");
-    }
-
-    #[test]
-    fn test_valid_process_names_from_processes() {
-        let mut procfile = Procfile::new();
-
-        procfile.insert("web", "rails -s");
-
-        let out = format_processes_for_log(&procfile);
-        assert_eq!(out, "web");
-
-        procfile.insert("worker", "rake sidekiq");
-
-        let out = format_processes_for_log(&procfile);
-        assert_eq!(out, "web, worker");
-    }
 }

src/main.rs

@@ -34,14 +34,12 @@ impl FromStr for Procfile {
     type Err = ProcfileParsingError;
 
     fn from_str(procfile_contents: &str) -> Result<Self, Self::Err> {
-        // 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");
+        let re_carriage_return_newline = Regex::new("\\r\\n?").expect("Clippy checked");
+        let re_multiple_newline = Regex::new("\\n*\\z").expect("Clippy checked");
 
         // 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:]]*")
-            .expect("Invalid Procfile regex");
+            .expect("Clippy checked");
 
         let procfile_contents = re_carriage_return_newline.replace_all(procfile_contents, "\n");
         let procfile_contents = re_multiple_newline.replace(&procfile_contents, "\n");

src/procfile.rs

@@ -21,8 +21,12 @@ fn test_web_and_worker_procfile() {
             assert_contains!(
                 context.pack_stdout,
                 indoc! {"
-                    [Discovering process types]
-                    Procfile declares types -> web, worker
+                    ## Procfile Buildpack
+
+                    - Processes from `Procfile`
+                      - web: `echo 'this is the web process!'`
+                      - worker: `echo 'this is the worker process!'`
+                    - Done (finished in < 0.1s)
                 "}
             );
 
@@ -51,8 +55,11 @@ fn test_worker_only_procfile() {
             assert_contains!(
                 context.pack_stdout,
                 indoc! {"
-                    [Discovering process types]
-                    Procfile declares types -> worker
+                    ## Procfile Buildpack
+
+                    - Processes from `Procfile`
+                      - worker: `echo 'this is the worker process!'`
+                    - Done (finished in < 0.1s)
                 "}
             );
 
@@ -79,8 +86,12 @@ fn test_multiple_non_web_procfile() {
             assert_contains!(
                 context.pack_stdout,
                 indoc! {"
-                    [Discovering process types]
-                    Procfile declares types -> worker, console
+                    ## Procfile Buildpack
+
+                    - Processes from `Procfile`
+                      - worker: `echo 'this is the worker process!'`
+                      - console: `echo 'this is the console process!'`
+                    - Done (finished in < 0.1s)
                 "}
             );
 
@@ -140,8 +151,11 @@ fn test_not_yaml_procfile() {
             assert_contains!(
                 context.pack_stdout,
                 indoc! {"
-                    [Discovering process types]
-                    Procfile declares types -> web
+                    ## Procfile Buildpack
+
+                    - Processes from `Procfile`
+                      - web: `echo foo: bar   `
+                    - Done (finished in < 0.1s)
                 "}
             );
             assert_contains!(context.pack_stdout, "Setting default process type 'web'");
@@ -162,8 +176,11 @@ fn test_empty_procfile() {
             assert_contains!(
                 context.pack_stdout,
                 indoc! {"
-                    [Discovering process types]
-                    Procfile declares types -> (none)
+                    ## Procfile Buildpack
+
+                    - Processes from `Procfile`
+                      - Empty file, no processes defined
+                    - Done (finished in < 0.1s)
                 "}
             );
             assert_contains!(context.pack_stdout, "no default process type");