-
Notifications
You must be signed in to change notification settings - Fork 331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Inconsistency with wait.PollImmediate and WaitForEndpointState #2407
Comments
cc @julz |
This issue is stale because it has been open for 90 days with no |
/remove-lifecycle stale |
This issue is stale because it has been open for 90 days with no |
/remove-lifecycle stale |
This issue is stale because it has been open for 90 days with no |
This issue or pull request is stale because it has been open for 90 days with no activity. This bot triages issues and PRs according to the following rules:
You can:
/lifecycle stale |
This issue is stale because it has been open for 90 days with no |
/lifecycle frozen |
/area test-and-release
/kind bug
Expected Behavior
wait.PollImmediate
should keep on polling if I pass response checkers throughWaitForEndpointState
https://github.com/knative/pkg/blob/main/test/request.go#L78 until the state is reached or timeout happens.Actual Behavior
Since the response checkers for example: https://github.com/knative/pkg/blob/main/test/spoof/response_checks.go#L33 returns true or error, it makes
wait.PollImmediate
stop polling, that is - only the first endpoint state is checked.Additional Info
Apologies if I am reading the code wrong, but if this really is an inconsistency, I think the return should be changed to
false, nil
(in the statuscode check function as well), this makes sense according to me because anywayWaitForEndpointState
is the only function which is propogating the response checkers to the Poll method, forCheckEndpointState
its defaulted totrue, nil
and it makes sense because this method's scope is to just check once. If this reasoning makes sense, I can create a PR.The text was updated successfully, but these errors were encountered: