feat: Adding error handling for discovery #4098
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis PR updates the error handling and initialization of the discovery process used by the CLI commands. Both the Changes
Sequence Diagram(s)sequenceDiagram
participant Command
participant Discovery
participant DepDiscovery
participant Logger
Command->>Discovery: NewDiscovery(WorkingDir)
Discovery->>Discovery: WithSuppressParseErrors()
Discovery->>DepDiscovery: Create dependency discovery instance
DepDiscovery-->>Discovery: Return discovery results or parse errors
alt Parse error occurs and suppression enabled
Discovery->>Logger: Log warning with error details
Discovery-->>Command: Continue execution without error propagation
else Valid results or unsuppressed error
Discovery-->>Command: Return result or error normally
end
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
…hat they'll get picked up
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/discovery/discovery.go (1)
546-565: Multi-iteration cycle removal strategy.This works well but might remove many configs in complex cycles. Consider logging each removal for clarity:
for range maxCycleChecks { if cfg, err = c.CycleCheck(); err == nil { break } + opts.Logger.Debugf("Removing discovered config at path due to cycle: %s", cfg.Path) c = c.RemoveByPath(cfg.Path) }This helps diagnose multi-cycle removal scenarios.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cli/commands/find/find.go(2 hunks)cli/commands/list/list.go(1 hunks)docs/_docs/04_reference/06-experiments.md(2 hunks)internal/discovery/discovery.go(13 hunks)internal/discovery/discovery_test.go(1 hunks)test/integration_parse_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.go`: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
**/*.go: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
cli/commands/list/list.gocli/commands/find/find.gointernal/discovery/discovery_test.gotest/integration_parse_test.gointernal/discovery/discovery.go
`docs/**/*.md`: Review the documentation for clarity, grammar, and spelling. Make sure that the documentation is easy to understand and follow. There is currently a migration under...
docs/**/*.md: Review the documentation for clarity, grammar, and spelling. Make sure that the documentation is easy to understand and follow. There is currently a migration underway from the Jekyll based documentation indocsto the Starlight + Astro based documentation indocs-starlight. Whenever changes are made to thedocsdirectory, ensure that an equivalent change is made in thedocs-starlightdirectory to keep thedocs-starlightdocumentation accurate.
docs/_docs/04_reference/06-experiments.md
🧬 Code Definitions (3)
cli/commands/list/list.go (1)
internal/discovery/discovery.go (1)
NewDiscovery(85-96)
cli/commands/find/find.go (1)
internal/discovery/discovery.go (1)
NewDiscovery(85-96)
test/integration_parse_test.go (1)
test/helpers/package.go (1)
RunTerragruntCommandWithOutput(827-831)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (34)
internal/discovery/discovery.go (20)
6-6: Imports look good.The new
ioimport is used for discarding output whensuppressParseErrorsis set, which helps reduce noise.
16-17: Logging dependency addition.The introduction of
github.com/gruntwork-io/terragrunt/pkg/logand its formatter is consistent with the new suppress-logging approach.
77-78: NewsuppressParseErrorsfield.This boolean flag cleanly aligns with the overarching design to optionally ignore parse errors during discovery.
137-142: Chainable method to toggle parse error suppression.This is consistent with the existing style of
With*methods, enabling selective error handling when discovering modules.
226-235: Conditional enabling of parse error suppression in dependency discovery.This implementation follows the new
WithSuppressParseErrorsapproach. It correctly ensures downstream dependency discovery inherits the same flag.
241-244: Warn-level logging for suppressed parse errors.Converting parse errors to warnings is aligned with the PR objective. No issues detected, and debug logs are used to capture full details.
248-257: Cycle detection and removal flow.Good approach to detect and remove cycles before finalizing discovered configs. The warning plus debug statements effectively communicate the issue and resolution steps.
267-273: ExtendedDependencyDiscoverywithsuppressParseErrors.Mirroring the pattern established in
Discoveryensures consistent error-suppression handling across both code paths.
275-281: Constructor signature forNewDependencyDiscovery.The initialization combines default values with the given config references. The pattern remains clear and maintains backward compatibility in other calls.
285-290: ChainableWithSuppressParseErrorsforDependencyDiscovery.Matches the style in the
Discoverystruct. Straightforward, no additional concerns noted.
292-297: ChainableWithDiscoverExternalDependencies.Again, follows the pattern of toggling features with a fluent API. Nicely done.
325-329: Early return for stack configs.This is consistent with current design assumptions that stacks do not hold direct dependencies. No concerns unless future requirements change.
331-341: Discarding output for parse operations.Excellent approach to prevent irrelevant parse logs from surfacing when
suppressParseErrorsis in effect.
354-363: Partial parse error handling.Ensure
config.PartialParseConfigFilewill always yield a non-nilcfgon partial parses. Otherwise, ignoring parse errors might mask deeper issues. If you’d like, I can generate a script to verify if partial parse commonly produces a nil config upon error.
375-375: Path resolution logic for dependencies.Using
filepath.Join(parseOpts.WorkingDir, depPath)keeps path references consistent and reduces relative path pitfalls.
384-384: Absolute path logic for dependency references.Similarly ensures paths remain consistent when building the dependency graph.
395-397: Early return for empty dependency sets.Saves time and avoids unnecessary processing when no dependencies are present.
448-450: Constructing external config stubs.Correctly sets up a minimal
DiscoveredConfigfor external references, ensuring they can still be tracked if needed.
480-491:RemoveByPathforDiscoveredConfigs.This helper is straightforward and will be especially useful for removing problematic entries encountered in cycle detection or error scenarios.
503-544: RevisedCycleCheckto return the first config in a cycle.Clear logic for cycle detection. The approach to return the offending config (
*DiscoveredConfig) alongside the error helps identify the cycle origin.cli/commands/list/list.go (1)
21-23: Suppressing parse errors during list discovery.Chaining
WithSuppressParseErrors()follows the new standard for graceful error handling without interrupting user workflows.cli/commands/find/find.go (2)
17-19: New chaining of suppression for the discovery object.This mirrors the
listcommand’s pattern, promoting uniform error-handling across CLI commands.
35-36: Warn instead of fail on discovery errors.Shifting from returning the error to issuing a warning aligns with the PR objective of allowing partial or best-effort results when parse errors occur.
internal/discovery/discovery_test.go (3)
380-381: Method signature update looks good.The test now properly captures both the configuration and error return values from the
CycleCheckmethod, aligning with the updated method signature.
384-384: Good assert addition for cycle detection.This assertion correctly verifies that when a cycle is detected, the configuration where the cycle was found is returned (not nil) alongside the error, which provides better debugging information.
387-387: Good assert addition for no cycle scenario.This assertion correctly verifies that when no cycle is detected, no configuration is returned (nil), which complements the error check on the previous line.
docs/_docs/04_reference/06-experiments.md (2)
169-170: Documentation update reflects completed features.The checkmarks now correctly indicate that error handling for broken configurations and integration tests for the
findcommand have been implemented, which aligns with the PR objective of enhancing error handling.
184-185: Documentation update reflects completed features.The checkmarks now correctly indicate that error handling for broken configurations and integration tests for the
listcommand have been implemented, which aligns with the PR objective of enhancing error handling.test/integration_parse_test.go (6)
3-6: Improved comment clarity about memory issues.The updated comments clearly explain the memory consumption problem and the need to run these tests in isolation, which is helpful for future developers who may encounter or need to address these issues.
25-31: Good addition of centralized problematic files list.Centralizing the list of known problematic files makes it easier to maintain and understand which files are expected to fail parsing, improving code maintainability.
66-69: Good memory management improvement.Explicitly triggering garbage collection after parsing each configuration file is a smart approach to reduce memory pressure, especially for these memory-intensive tests.
74-116: Well-implemented test for basic command functionality.This new test properly validates that both
findandlistcommands work correctly without additional flags. It also verifies that the dependency order is as expected (a-dependent comes before b-dependency in the output).
118-160: Well-implemented test for DAG sorting functionality.This test correctly validates that when using the
--dagflag, the dependency order is reversed compared to the default alphabetical sorting (b-dependency now comes before a-dependent in the output), which is the expected behavior for DAG-based sorting.
162-204: Well-implemented test for DAG with external dependencies.This test properly validates the combined functionality of
--dagand--externalflags, ensuring that dependency order is maintained correctly even when external dependencies are included.
| break | ||
| } | ||
|
|
||
| c = c.RemoveByPath(cfg.Path) |
There was a problem hiding this comment.
It shouldn't ever have nil, but I can add a check.
Description
Adds error handling for discovery.
This is important, as it allows the
listandfindcommands to operate without failure when running against directories of configurations with requirements like authentication, etc. that aren't relevant to the discovery process.TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Updated
discoveryto handle errors gracefully.Summary by CodeRabbit
New Features
Documentation
Tests