-
Notifications
You must be signed in to change notification settings - Fork 39
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
Fix DeploymentConfig idling #614
Fix DeploymentConfig idling #614
Conversation
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.
Thanks for fixing it!
@@ -197,6 +197,7 @@ func (a *IdleablePayloadAssertion) DeploymentConfigScaledDown(deployment *opensh | |||
err := a.client.Get(context.TODO(), types.NamespacedName{Name: deployment.Name, Namespace: deployment.Namespace}, d) | |||
require.NoError(a.t, err) | |||
assert.Equal(a.t, int32(0), d.Spec.Replicas) | |||
assert.False(a.t, d.Spec.Paused) // DeploymentConfig should be unpaused when scaling down so that the replicas update can be rolled out |
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.
Shouldn't we set it to true and then check it was reset to false? Otherwise the test would pass even if we don't reset it, right?
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.
sure, added 9563449
@@ -197,6 +197,7 @@ func (a *IdleablePayloadAssertion) DeploymentConfigScaledDown(deployment *opensh | |||
err := a.client.Get(context.TODO(), types.NamespacedName{Name: deployment.Name, Namespace: deployment.Namespace}, d) | |||
require.NoError(a.t, err) | |||
assert.Equal(a.t, int32(0), d.Spec.Replicas) | |||
assert.False(a.t, d.Spec.Paused) // DeploymentConfig should be unpaused when scaling down so that the replicas update can be rolled out |
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.
should it be set d.Spec.Paused
to true
in the tests, before running the idler?
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.
sure, added 9563449
Quality Gate passedIssues Measures |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #614 +/- ##
=======================================
Coverage 81.59% 81.59%
=======================================
Files 29 29
Lines 3292 3293 +1
=======================================
+ Hits 2686 2687 +1
Misses 457 457
Partials 149 149
|
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.
Thanks!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, rajivnathan, xcoulon 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 |
304ca61
into
codeready-toolchain:master
fixes https://issues.redhat.com/browse/SANDBOX-942
updated e2e test: codeready-toolchain/toolchain-e2e#1096