fix: replace panics with proper error returns in dockerfile2dot#808
fix: replace panics with proper error returns in dockerfile2dot#808patrickhoefler merged 6 commits intomainfrom
Conversation
patrickhoefler
commented
Mar 12, 2026
patrickhoefler
commented
- LoadAndParseDockerfile: return errors instead of panicking on filepath.Abs failure and non-ErrNotExist filesystem errors
- getWaitForNodeID: return (string, map[string]string, error) instead of panicking when a node ID cannot be resolved
- BuildDotFile: change signature to (string, error) and surface all gographviz errors via a first-error collector; extract helper functions to keep cyclomatic complexity within limits
- addEdgesForStage, addLegend: return error and propagate
- LoadAndParseDockerfile: return errors instead of panicking on filepath.Abs failure and non-ErrNotExist filesystem errors - getWaitForNodeID: return (string, map[string]string, error) instead of panicking when a node ID cannot be resolved - BuildDotFile: change signature to (string, error) and surface all gographviz errors via a first-error collector; extract helper functions to keep cyclomatic complexity within limits - addEdgesForStage, addLegend: return error and propagate Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates dockerfile2dot to avoid panics by returning and propagating errors through Dockerfile loading and DOT graph construction, and adjusts the CLI/tests to handle the new error-returning APIs.
Changes:
- Replace panic paths in Dockerfile loading with proper
errorreturns. - Change DOT generation (
BuildDotFile, legend/edge helpers, node resolution) to return errors and propagate gographviz failures. - Update CLI and unit tests to use the new
BuildDotFile() (string, error)signature.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/dockerfile2dot/load.go | Return errors instead of panicking when resolving absolute paths / read failures. |
| internal/dockerfile2dot/build.go | Refactor DOT generation to propagate errors and remove panics from node resolution. |
| internal/dockerfile2dot/build_test.go | Update tests to handle the new (string, error) return signature. |
| internal/cmd/root.go | Update CLI to handle BuildDotFile errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Validate numeric stage index is in range before indexing Stages slice - Validate stage has at least one layer before using Layers[-1] when layers mode is enabled; same check for stage-name resolution path - Add TestBuildDotFileErrors covering: unresolvable WaitFor ID, out-of-range numeric stage reference (flat and layers mode) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- arg.Label values come from Dockerfile ARG instructions and can
contain spaces, '=', and other non-identifier characters; wrap them
in explicit DOT quotes ("...") the same way stage layer labels are
handled elsewhere in the file
- Quote the "Before First Stage" cluster subgraph label explicitly for
consistency; gographviz.Escape would auto-quote it anyway but being
explicit matches the pattern used for other multi-word labels
- Cluster stage labels (getStageLabel) are left to gographviz.Escape
since it correctly quotes names with hyphens while leaving plain
identifiers unquoted, matching the existing golden test output
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…or message - Remove the set func(error) parameter from addStageWithLayers; give it its own var/set pair and return graphErr instead of always nil, making the error propagation explicit and the if err != nil check meaningful - Use %q and add context to the unresolvable node ID error message so callers can distinguish whitespace/special-char identifiers and know what was expected Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Check graphErr before each helper call in BuildDotFile so that a SetName/AddAttr failure is not masked by a later helper's error - Check graphErr before addEdgesForStage in addStages so that an earlier node/subgraph error is returned first - Include the input filename in the filepath.Abs error message so users can see which path failed to resolve Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Stage names can contain hyphens, dots, or other non-identifier characters that require quoting in DOT format. Apply the same "\"...\"" pattern used for node labels throughout the file so the quoting is explicit and not reliant on gographviz auto-escaping. Update the raw DOT expectations in the layers flag test cases to reflect the new quoted form (label="ubuntu", label="release"). Canon test expectations are unchanged because graphviz strips redundant quotes from simple identifiers when normalising to canonical format. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
