Return errors instead of accumulating diagnostics#5721
Draft
denik wants to merge 6 commits into
Draft
Conversation
Replace the logdiag-based error accumulation in the bundle pipeline with idiomatic Go error returns. Mutators, phases, deploy, statemgmt and the direct engine now return error; logdiag.LogError and logdiag.HasError are removed. A diag.Diagnostic implements error and diag.Diagnostics.Error() combines all error-severity diagnostics (via errors.Join) so a returned error still unpacks into separate diagnostic blocks and reports the error count when rendered. logdiag.FlushError renders errors at their source (before slow deferred cleanup) and is idempotent, with RenderAndReturnError as the boundary fallback. Co-authored-by: Isaac
Mutators that report several errors now log each diagnostic immediately and return the ErrAlreadyPrinted sentinel, via the new logdiag.Flush(ctx, diags) helper, instead of combining them into an errors.Join. This keeps errors adjacent to their warnings in source order and lets the error count fall out of the logdiag counter. Test helpers skip appending the returned error when it is ErrAlreadyPrinted (those diagnostics are already collected). logdiag tracks the first error summary again so deploy telemetry stays meaningful when the returned error is the opaque sentinel. Co-authored-by: Isaac
ValidateDeploymentFields (added on main) now returns error via logdiag.Flush. Update the IsolatedContext test and ValidateDeploymentFields test to the new API (run the mutator via bundle.Apply / assert the returned error). The yaml-sync config-snapshot warning now carries the specific failure instead of a generic "state conversion failed" wrapper. Co-authored-by: Isaac
Audit every mutator that reported multiple errors before the refactor and restore that behavior where it had regressed to first-error-only. Sequential mutators accumulate diagnostics and return logdiag.Flush(ctx, diags) (log each, return ErrAlreadyPrinted); the read-only parallel validators return diags.Error() so ApplyParallel joins them deterministically. run_as and validate_target_mode already preserved all errors via diag.Diagnostics.Error() (errors.Join); apply_presets and set_variables only ever produced a single error and are unchanged. Co-authored-by: Isaac
Co-authored-by: Isaac
Contributor
Approval status: pending
|
Collaborator
Integration test reportCommit: 015c702
21 interesting tests: 13 SKIP, 7 KNOWN, 1 RECOVERED
Top 4 slowest tests (at least 2 minutes):
|
Full golangci-lint (errcheck) flagged callers of ApplyFuncContext and the phases.* functions in benchmark/test helpers that ignored the now-returned error. Check them (require.NoError, or _ = in benchmark loops). Co-authored-by: Isaac
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.
Replace the logdiag-based diagnostics accumulation in the bundle pipeline with idiomatic Go error returns.
error;logdiag.LogErrorandlogdiag.HasErrorare removed.diag.Diagnosticimplementserror. Errors render vialogdiag.FlushErrorat their source (idempotent), so the user sees them before slow deferred cleanup. Mutators that report multiple errors log each vialogdiag.Flush(ctx, diags)(or returndiags.Error()from parallel validators).logdiag.LogDiag.The only acceptance output changes are the redundant generic "planning failed" wrapper being dropped (the specific resource errors are still shown) and multi-error blocks now rendering as separate
Error:entries.This pull request and its description were written by Isaac.