-
Notifications
You must be signed in to change notification settings - Fork 73
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
Runtime 15 e2e #1043
Runtime 15 e2e #1043
Conversation
Skipping CI for Draft Pull Request. |
@@ -440,9 +440,10 @@ func (a *Awaitility) CreateNamespace(t *testing.T, name string) { | |||
func (a *Awaitility) WaitForDeploymentToGetReady(t *testing.T, name string, replicas int, criteria ...DeploymentCriteria) *appsv1.Deployment { | |||
t.Logf("waiting until deployment '%s' in namespace '%s' is ready", name, a.Namespace) | |||
deployment := &appsv1.Deployment{} | |||
err := wait.Poll(a.RetryInterval, 6*a.Timeout, func() (done bool, err error) { | |||
err := wait.PollUntilContextTimeout(context.TODO(), a.RetryInterval, 6*a.Timeout, true, func(ctx context.Context) (done bool, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Poll is deprecated
…Get, revert retryInterval
@@ -365,11 +364,12 @@ func TestProxyFlow(t *testing.T) { | |||
require.NoError(t, err) | |||
err = proxyCl.Create(context.TODO(), appToCreate) | |||
|
|||
// then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment block is removed as it is no longer valid. Maintaining history here might be confusing
… into runtime_15_e2e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I left a few minor comments though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks for addressing the comments.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, ranakan19 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Co-authored-by: Alexey Kazakov <[email protected]>
Quality Gate passedIssues Measures |
Quality Gate passedIssues Measures |
Changes this PR includes:
- With the changes to rest mapper that provided client for discovery information to do REST mappings, breaks the use case of checking available APIs via
k8s.io/apimachinery/pkg/api/errors.IsNotFound
andk8s.io/apimachinery/pkg/api/meta.IsNoMatchError
. This means can not directly check for those errors, but should expect server error. This only affected proxy tests and the tests now check for error contains. Can find more details in this issue- Tried to propogate the context from PollUntillContextTimeout to the api calls within the condition function but it fails with
would exceed context deadline
as the last error instead of the expectedcontext deadline exceeded
error. Please refer this comment for more details. And other PR which use the same solution of intentionally pass a separate context to api calls in the consition function.- PollUntillContextTimeout will no longer return
ErrWaitTimeout
- So replaces errTimeout checks with context deadline checks. [refer this PR for more information if needed]