ARO-26564: fix: include condition name in provisioning timeout errors#4841
Merged
Conversation
d83bb4c to
55e8ddf
Compare
iormungand
approved these changes
May 20, 2026
Collaborator
iormungand
left a comment
There was a problem hiding this comment.
- Confirmed all Conditions in install.go are now covered
- Good error messages
- Added better information for any future Conditions missing from timeoutConditionErrors map
- Tests all run
LGTM
Collaborator
|
Looks like perhaps this ticket is asking for the wrong thing: These errors are not actionable by the user - incorrect configuration should be caught by preflight, rather than once they have timed out. Surfacing details could make the problem worse if they try to use an LLM. Looks like we really just need to write a log line, and put comments in the code to document the exception being made. |
Collaborator
Author
|
/help |
When a condition step times out and is not in the timeoutConditionErrors map, the error returned was the raw "timed out waiting for the condition" with no indication of which condition failed. - Wrap the original error with the condition function name in enrichConditionTimeoutError() when the function is not found in the timeoutConditionErrors map https://github.com/Azure/ARO-RP/blob/fc8e2af90c55d096f6f0db89892d1e543925b4f6/pkg/util/steps/condition.go#L26 - Use `fmt.Errorf("condition step failed: %s: %w", ...)` rather than `fmt.Errorf("timed out waiting for condition %s", ...)` because using a distinct prefix avoids the redundant output of `timed out waiting for condition X: timed out waiting for the condition` - Update unit and integration tests to match the new error format Signed-off-by: Nikhil Thomas <nikthoma@redhat.com>
55e8ddf to
1dce17b
Compare
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
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.
Which issue this PR addresses: ARO-26564
Fixes https://redhat.atlassian.net/browse/ARO-26564
Fixes
What this PR does / why we need it:
When a condition step times out and is not in the
timeoutConditionErrors map, the error returned was the raw
"timed out waiting for the condition" with no indication of
which condition failed.
enrichConditionTimeoutError() when the function is not found
in the timeoutConditionErrors map
ARO-RP/pkg/util/steps/condition.go
Line 26 in fc8e2af
fmt.Errorf("condition step failed: %s: %w", ...)ratherthan
fmt.Errorf("timed out waiting for condition %s", ...)because using a distinct prefix avoids the redundant output of
timed out waiting for condition X: timed out waiting for the conditionTest plan for issue:
How did you test that this PR works?
Is there any documentation that needs to be updated for this PR?
How do you know this will function as expected in production?