WINC-607: [e2e] Skip MachineSet update in deletion test #899
Conversation
This commit skips the MachineSet update in the Windows node deletion test if no MachineSet is found with the Windows label. This is a corner case for platform-agnostic infrastructure and e2e tests executions where no instances are configured via MachineController, for example: `hack/run-ci-e2e-test.sh -m 0`.
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
/test ? |
|
@jrvaldes: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jrvaldes The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test aws-e2e-operator |
|
/test lint |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@jrvaldes: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/cherry-pick release-4.10 |
|
@jrvaldes: once the present PR merges, I will cherry-pick it on top of release-4.10 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherry-pick community-4.9 |
|
@jrvaldes: once the present PR merges, I will cherry-pick it on top of community-4.9 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherry-pick release-4.9 |
|
@jrvaldes: once the present PR merges, I will cherry-pick it on top of release-4.9 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherry-pick community-4.8 |
|
@jrvaldes: once the present PR merges, I will cherry-pick it on top of community-4.8 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherry-pick release-4.8 |
|
@jrvaldes: once the present PR merges, I will cherry-pick it on top of release-4.8 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| @@ -115,7 +115,10 @@ func testWindowsNodeDeletion(t *testing.T) { | |||
| testCtx, err := NewTestContext() | |||
| require.NoError(t, err) | |||
|
|
|||
| // set excepted node count to zero, since all Windows Nodes should be deleted in the test | |||
| expectedNodeCount := int32(0) | |||
There was a problem hiding this comment.
nit: I'd prefer this initialized closer to its usage (around line 132)
|
|
||
| // skip the scale down step if there is no MachineSet with Windows label | ||
| if windowsMachineSetWithLabel != nil { | ||
| // Scale the Windows MachineSet to 0 |
There was a problem hiding this comment.
This comment was more meaningful where we used to set expectedNodeCount := int32(0) next line. It would be good if you can move it back here.
There was a problem hiding this comment.
expectedNodeCount := int32(0) does not scale Windows MachineSet to 0, testCtx.client.Machine.MachineSets(clusterinfo.MachineAPINamespace).Update(...) does.
In addition, expectedNodeCount is used outside of the if block, so moved it up to avoid duplication.
There was a problem hiding this comment.
but we are setting it in windowsMachineSetWithLabel.Spec.Replicas = &expectedNodeCount which is used in update call.
anyways this is very minor.
|
/lgtm |
|
@jrvaldes please run the platform none test on this PR to ensure the correct behavior is being seen |
|
/close Closing this PR to cover all e2e test adjustments for |
|
@jrvaldes: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This PR skips the MachineSet update in the Windows node deletion
test if no MachineSet is found with the Windows label. This is a
corner case for platform-agnostic infrastructure and e2e tests
executions where no instances are configured via MachineController, for
example:
hack/run-ci-e2e-test.sh -m 0.