You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
In #745 I started to critique the command execution, which isn't the primary intent of that PR. I'm pulling out the comments to save for future work.
The goals are to:
Ensure all errors stemming from running commands contain maximum information available to assist with debugging. This should help both buildpack users and maintainers.
Reduce/remove debugging errors stemming from a difference in what is displayed versus what was executed.
Command execution checklist
Here's a checklist of requirements for running commands:
An exact representation of the command being run should appear in logs, or the error, or both
Make non-zero status an error
If a command representation is output to the logs, changes to the command should be visible in the output (the display and command being run should come from a single point of truth).
Stdout and Stderr from the command should appear in logs, or the error, but NOT both (if it runs).
Stream output or print a progress indicator for commands that may take awhile or that access the network
You don't HAVE to use it but...you get a lot of these more-or-less out of the box when using fun_run. The current state of command execution leaves a lot of subtle gotchas. The goal in making these shared libs is so that we learn from our own experiences, translate that into tooling to make the most common cases easier, and push those best practices out to the community through that tooling.
Prior comments
Here are prior comments extracted from a previous review, it's more or less a re-hash of the checklist from above but commented on specific command invocations:
There's a partial command emitted earlier but on failure we don't have the full command that was run, needed for debugging. Please add it or emit the entire command. (Use unified Heroku buildpack output style #745 (comment))
Request: Log the command based off of what's going to be run. In this case the logging and the command can differ, and actually do here. You're running with internal_maven_options but are not printing them. Also you're using maven_options.chain() below but not here. (Use unified Heroku buildpack output style #745 (comment))
You're printing exactly what you're going to run here (yay!). I still request to refactor based off of a single source of truth such that someone changing the command arguments won't accidentally diverge from what's being printed. (Use unified Heroku buildpack output style #745 (comment))
Requested
Go through all places where commands are being invoked
Fill out the checklist for each
Modify the implementation until all items are complete
Open a PR focused on command execution and display
Optional:
Suggest changes to the checklist: Additions, modifications, etc.
The text was updated successfully, but these errors were encountered:
In #745 I started to critique the command execution, which isn't the primary intent of that PR. I'm pulling out the comments to save for future work.
The goals are to:
Command execution checklist
Here's a checklist of requirements for running commands:
You don't HAVE to use it but...you get a lot of these more-or-less out of the box when using fun_run. The current state of command execution leaves a lot of subtle gotchas. The goal in making these shared libs is so that we learn from our own experiences, translate that into tooling to make the most common cases easier, and push those best practices out to the community through that tooling.
Prior comments
Here are prior comments extracted from a previous review, it's more or less a re-hash of the checklist from above but commented on specific command invocations:
Requested
Optional:
The text was updated successfully, but these errors were encountered: