fix: wait works when no conditions set - #40
Merged
Conversation
Signed-off-by: Calum Murray <cmurray@redhat.com>
📝 WalkthroughWalkthroughThe PR modifies the kubernetes.wait operation to make the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/extension/wait.go (1)
60-69:⚠️ Potential issue | 🟠 MajorDon’t mask persistent
Geterrors as “not found” in existence mode.In no-condition mode, every
Geterror is retried and timeout is reported as “not found”. That hides real causes (e.g., auth/API errors) and makes debugging hard. Preserve/report the lastGeterror (or fail fast for clearly non-retryable errors).Suggested diff
var lastStatus string + var lastGetErr error err = wait.PollUntilContextTimeout(ctx, time.Second, timeout, true, func(ctx context.Context) (bool, error) { obj, getErr := e.client.Get(ctx, gvr, ref.name, ref.namespace) if getErr != nil { + lastGetErr = getErr return false, nil // Keep polling on transient errors } @@ if err != nil { if condition == "" { @@ return sdk.FailureWithMessage( - fmt.Sprintf("Resource %s/%s not found", ref.kind, ref.name), - fmt.Errorf("timed out waiting for %s/%s to exist", ref.kind, ref.name), + fmt.Sprintf("Resource %s/%s not ready", ref.kind, ref.name), + fmt.Errorf("timed out waiting for %s/%s to exist (last get error: %v)", ref.kind, ref.name, lastGetErr), ), nil }Also applies to: 97-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/extension/wait.go` around lines 60 - 69, In the existence/no-condition branch inside the wait.PollUntilContextTimeout loop (where you call e.client.Get and check if condition == ""), do not always swallow getErr and return false,nil; instead detect and immediately fail on non-retryable errors (e.g., auth/API errors) by returning false,getErr (or use apierrors.IsNotFound to treat NotFound as retryable), and for retryable errors keep track of the last getErr and propagate it after the poll times out so callers see the real failure; apply the same change pattern to the similar logic around lines 97-105 so persistent Get errors are preserved/reported rather than masked as “not found.”
🧹 Nitpick comments (2)
pkg/extension/wait_test.go (1)
173-181: Consider asserting response messages for new branches.The new behavior has distinct success/failure messages for existence vs condition waits; asserting them would harden the contract and catch regressions beyond the boolean outcome.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/extension/wait_test.go` around lines 173 - 181, The test currently only checks result.Success for handleWait; add assertions on the response message to cover new branches (existence vs condition waits). In the pkg/extension/wait_test.go test cases where you call ext.handleWait(context.Background(), req) (using sdk.OperationRequest and checking result.Success), also assert result.Message (or result.ErrorMessage if applicable) matches the expected string for each case (e.g., "resource exists" vs "condition met" or the appropriate failure text) so both positive and negative branches are validated.pkg/extension/operations.go (1)
43-47: Clarifywaitoperation docs for existence-only mode.Line 73 now allows
conditionto be omitted, but Line 43 and Line 46 still describe condition-only waiting. Please update those descriptions so callers know omission means existence wait.Suggested doc-only diff
- sdk.WithDescription("Wait for a condition on a Kubernetes resource"), + sdk.WithDescription("Wait for a condition on a Kubernetes resource, or for resource existence if condition is omitted"), ... - Description: "Resource reference with condition to wait for", + Description: "Resource reference to wait on (condition optional; existence check when omitted)",Also applies to: 73-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/extension/operations.go` around lines 43 - 47, Update the operation descriptions to document that omitting the "condition" field switches the wait to existence-only behavior: modify the text passed to sdk.WithDescription(...) and the jsonschema.Schema Description for WithParams so they state that callers can omit "condition" to wait only for resource existence (rather than waiting for a specific condition), and ensure the "condition" property description in the Properties map (the jsonschema.Schema entry for "condition") is updated similarly to reflect optional existence-only behavior; reference sdk.WithDescription, sdk.WithParams, jsonschema.Schema and the "condition" property to locate and update the strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/extension/wait.go`:
- Around line 60-69: In the existence/no-condition branch inside the
wait.PollUntilContextTimeout loop (where you call e.client.Get and check if
condition == ""), do not always swallow getErr and return false,nil; instead
detect and immediately fail on non-retryable errors (e.g., auth/API errors) by
returning false,getErr (or use apierrors.IsNotFound to treat NotFound as
retryable), and for retryable errors keep track of the last getErr and propagate
it after the poll times out so callers see the real failure; apply the same
change pattern to the similar logic around lines 97-105 so persistent Get errors
are preserved/reported rather than masked as “not found.”
---
Nitpick comments:
In `@pkg/extension/operations.go`:
- Around line 43-47: Update the operation descriptions to document that omitting
the "condition" field switches the wait to existence-only behavior: modify the
text passed to sdk.WithDescription(...) and the jsonschema.Schema Description
for WithParams so they state that callers can omit "condition" to wait only for
resource existence (rather than waiting for a specific condition), and ensure
the "condition" property description in the Properties map (the
jsonschema.Schema entry for "condition") is updated similarly to reflect
optional existence-only behavior; reference sdk.WithDescription, sdk.WithParams,
jsonschema.Schema and the "condition" property to locate and update the strings.
In `@pkg/extension/wait_test.go`:
- Around line 173-181: The test currently only checks result.Success for
handleWait; add assertions on the response message to cover new branches
(existence vs condition waits). In the pkg/extension/wait_test.go test cases
where you call ext.handleWait(context.Background(), req) (using
sdk.OperationRequest and checking result.Success), also assert result.Message
(or result.ErrorMessage if applicable) matches the expected string for each case
(e.g., "resource exists" vs "condition met" or the appropriate failure text) so
both positive and negative branches are validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 277271e1-ab40-4aa2-ad82-3fc7dfa2266c
📒 Files selected for processing (3)
pkg/extension/operations.gopkg/extension/wait.gopkg/extension/wait_test.go
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.
Resolves #39
Summary by CodeRabbit
conditionfield is now optional for the wait operation; resources can be validated by checking existence alone.