fix: Fixing report run duplication error#5252
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughAdds a logger parameter to Report API methods (AddRun, EnsureRun, EndRun), propagates the logger to all call sites, and replaces previously-silent error paths with explicit error-checked logging in dependency discovery and run/error handling flows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25–35 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
yhakbar
force-pushed
the
fix/fixing-report-duplication
branch
from
8c7246a to
cc16dce
Compare
yhakbar
force-pushed
the
fix/fixing-report-duplication
branch
from
cc16dce to
0969ec6
Compare
denis256
reviewed
Dec 18, 2025
yhakbar
force-pushed
the
fix/fixing-report-duplication
branch
from
0969ec6 to
4f421ed
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/runner/common/unit_runner.go (1)
83-106: Minor optimization opportunity: path is cleaned twice.The
unitPathis cleaned on line 60 and again on line 84. Since the path doesn't change between these calls, you could store the cleaned path in a variable to avoid redundant cleaning.🔎 View suggested refactor:
// Only create report entries if report is not nil if r != nil { unitPath := runner.Unit.AbsolutePath() unitPath = util.CleanPath(unitPath) if _, err := r.EnsureRun(runner.Unit.Execution.Logger, unitPath); err != nil { return err } + + // Store cleaned path to avoid recleaning later + defer func(cleanedPath string) { + if runErr != nil { + if endErr := r.EndRun( + runner.Unit.Execution.Logger, + cleanedPath, + report.WithResult(report.ResultFailed), + report.WithReason(report.ReasonRunError), + report.WithCauseRunError(runErr.Error()), + ); endErr != nil { + runner.Unit.Execution.Logger.Errorf("Error ending run for unit %s: %v", cleanedPath, endErr) + } + } else { + if endErr := r.EndRun( + runner.Unit.Execution.Logger, + cleanedPath, + report.WithResult(report.ResultSucceeded), + ); endErr != nil { + runner.Unit.Execution.Logger.Errorf("Error ending run for unit %s: %v", cleanedPath, endErr) + } + } + }(unitPath) } // Use a unit-scoped detailed exit code so retries in this unit don't clobber global state globalExitCode := tf.DetailedExitCodeFromContext(ctx) var unitExitCode tf.DetailedExitCode ctx = tf.ContextWithDetailedExitCode(ctx, &unitExitCode) runErr := opts.RunTerragrunt(ctx, runner.Unit.Execution.Logger, opts, r) // Only merge the final unit exit code when the unit run completed without error // and the exit code isn't stuck at 1 from a prior retry attempt. if runErr == nil && globalExitCode != nil && unitExitCode.Get() != tf.DetailedExitCodeError { globalExitCode.Set(unitExitCode.Get()) } - - // End the run with appropriate result (only if report is not nil) - if r != nil { - unitPath := runner.Unit.AbsolutePath() - unitPath = util.CleanPath(unitPath) - - if runErr != nil { - if endErr := r.EndRun( - runner.Unit.Execution.Logger, - unitPath, - report.WithResult(report.ResultFailed), - report.WithReason(report.ReasonRunError), - report.WithCauseRunError(runErr.Error()), - ); endErr != nil { - runner.Unit.Execution.Logger.Errorf("Error ending run for unit %s: %v", unitPath, endErr) - } - } else { - if endErr := r.EndRun( - runner.Unit.Execution.Logger, - unitPath, - report.WithResult(report.ResultSucceeded), - ); endErr != nil { - runner.Unit.Execution.Logger.Errorf("Error ending run for unit %s: %v", unitPath, endErr) - } - } - } return runErrAlternatively, if you prefer to keep the current structure, simply reuse the cleaned path:
// Only create report entries if report is not nil + var cleanedUnitPath string if r != nil { unitPath := runner.Unit.AbsolutePath() - unitPath = util.CleanPath(unitPath) + cleanedUnitPath = util.CleanPath(unitPath) - if _, err := r.EnsureRun(runner.Unit.Execution.Logger, unitPath); err != nil { + if _, err := r.EnsureRun(runner.Unit.Execution.Logger, cleanedUnitPath); err != nil { return err } } // Use a unit-scoped detailed exit code so retries in this unit don't clobber global state globalExitCode := tf.DetailedExitCodeFromContext(ctx) var unitExitCode tf.DetailedExitCode ctx = tf.ContextWithDetailedExitCode(ctx, &unitExitCode) runErr := opts.RunTerragrunt(ctx, runner.Unit.Execution.Logger, opts, r) // Only merge the final unit exit code when the unit run completed without error // and the exit code isn't stuck at 1 from a prior retry attempt. if runErr == nil && globalExitCode != nil && unitExitCode.Get() != tf.DetailedExitCodeError { globalExitCode.Set(unitExitCode.Get()) } // End the run with appropriate result (only if report is not nil) if r != nil { - unitPath := runner.Unit.AbsolutePath() - unitPath = util.CleanPath(unitPath) - if runErr != nil { if endErr := r.EndRun( runner.Unit.Execution.Logger, - unitPath, + cleanedUnitPath, report.WithResult(report.ResultFailed), report.WithReason(report.ReasonRunError), report.WithCauseRunError(runErr.Error()), ); endErr != nil { - runner.Unit.Execution.Logger.Errorf("Error ending run for unit %s: %v", unitPath, endErr) + runner.Unit.Execution.Logger.Errorf("Error ending run for unit %s: %v", cleanedUnitPath, endErr) } } else { if endErr := r.EndRun( runner.Unit.Execution.Logger, - unitPath, + cleanedUnitPath, report.WithResult(report.ResultSucceeded), ); endErr != nil { - runner.Unit.Execution.Logger.Errorf("Error ending run for unit %s: %v", unitPath, endErr) + runner.Unit.Execution.Logger.Errorf("Error ending run for unit %s: %v", cleanedUnitPath, endErr) } } } return runErr
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/discovery/dependencydiscovery.go(1 hunks)internal/report/report.go(6 hunks)internal/report/report_test.go(33 hunks)internal/runner/common/unit_runner.go(3 hunks)internal/runner/runnerpool/runner.go(5 hunks)options/options.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/runner/runnerpool/runner.go
- internal/report/report.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
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.
Files:
internal/runner/common/unit_runner.gooptions/options.gointernal/report/report_test.gointernal/discovery/dependencydiscovery.go
🧠 Learnings (2)
📚 Learning: 2025-02-10T13:36:19.542Z
Learnt from: levkohimins
Repo: gruntwork-io/terragrunt PR: 3723
File: cli/commands/stack/action.go:160-160
Timestamp: 2025-02-10T13:36:19.542Z
Learning: The project uses a custom error package `github.com/gruntwork-io/terragrunt/internal/errors` which provides similar functionality to `fmt.Errorf` but includes stack traces. Prefer using this package's error functions (e.g., `errors.Errorf`, `errors.New`) over the standard library's error handling.
Applied to files:
options/options.gointernal/report/report_test.go
📚 Learning: 2025-08-19T16:05:54.723Z
Learnt from: Resonance1584
Repo: gruntwork-io/terragrunt PR: 4683
File: go.mod:86-90
Timestamp: 2025-08-19T16:05:54.723Z
Learning: When analyzing Go module dependencies for removal, always check for both direct imports and API usage across all Go files in the repository, not just a quick search. The github.com/mattn/go-zglob library is used for filesystem walking and glob expansion in multiple Terragrunt files including util/file.go, format commands, and AWS provider patch functionality.
Applied to files:
internal/report/report_test.go
🧬 Code graph analysis (4)
internal/runner/common/unit_runner.go (2)
internal/component/unit.go (1)
Unit(22-33)internal/report/report.go (2)
WithResult(257-261)ResultSucceeded(54-54)
options/options.go (2)
internal/report/report.go (1)
Report(16-23)util/file.go (1)
CleanPath(637-639)
internal/report/report_test.go (2)
test/helpers/logger/logger.go (1)
CreateLogger(9-14)internal/report/report.go (11)
WithResult(257-261)Report(16-23)WithReason(264-268)ReasonRunError(63-63)ResultExcluded(57-57)WithCauseRetryBlock(274-276)ResultSucceeded(54-54)ReasonRetrySucceeded(61-61)ResultEarlyExit(56-56)WithCauseExcludeBlock(290-292)ReasonAncestorError(67-67)
internal/discovery/dependencydiscovery.go (1)
internal/report/report.go (4)
WithResult(257-261)ResultExcluded(57-57)WithReason(264-268)ReasonExcludeExternal(66-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint / lint
- GitHub Check: license_check / License Check
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (7)
internal/discovery/dependencydiscovery.go (1)
293-307: LGTM! Error handling for excluded dependencies is well implemented.The changes properly handle errors when creating and ending runs for excluded external dependencies by logging them without stopping execution, which is appropriate for reporting purposes. The use of cleaned absolute paths ensures consistency across the reporting flow.
internal/report/report_test.go (3)
54-67: LGTM! Logger integration is correct.The test properly creates a logger and passes it to all
AddRuncalls, consistent with the new API signature.
116-196: LGTM! Logger properly propagated through test setup.The test correctly creates a logger and passes it through both the setup function and all report method calls, ensuring consistent logging context.
457-526: LGTM! Test setup functions properly updated.The setup function signatures correctly accept a logger parameter, and the logger is properly propagated to all report method calls within the test scenarios.
options/options.go (2)
692-697: LGTM! Path normalization is correctly implemented.The two-step process of converting to an absolute path followed by cleaning ensures consistent path formatting for reporting. Error handling is appropriate.
725-740: LGTM! Logger and path handling are consistent across error flows.Both the ignore and retry error handling paths correctly propagate the logger to
EnsureRunandEndRun, and consistently use the cleanedreportDirfor all reporting operations. The error handling ensures any reporting failures are properly caught and returned.Also applies to: 758-771
internal/runner/common/unit_runner.go (1)
58-64: LGTM! Run creation is correctly implemented.The code properly checks for a nil report, cleans the unit path, and calls
EnsureRunwith the unit's logger. The return value is appropriately discarded since it's not needed, and error handling is correct.
denis256
approved these changes
Dec 18, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes issue with duplicate reports causing failures in runs.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.