test: add rollout strategy transition E2Es for staged update run #305
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Signed-off-by: Arvind Thirumurugan <arvindth@microsoft.com>
Signed-off-by: Arvind Thirumurugan <arvindth@microsoft.com>
Signed-off-by: Arvind Thirumurugan <arvindth@microsoft.com>
Arvindthiru
force-pushed
the
stagedUpdateRunE2E2
branch
from
3558bc3 to
710b929
Compare
Signed-off-by: Arvind Thirumurugan <arvindth@microsoft.com>
| []string{resourceSnapshotIndex1st, resourceSnapshotIndex1st, resourceSnapshotIndex1st}, []bool{true, true, true}, nil, nil) | ||
| Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP %s status for external strategy", crpName) | ||
| It("Should update crp status to reflect external rollout strategy with new observed generation and no other change", func() { | ||
| crpStatusUpdatedActual := crpStatusUpdatedActual(workResourceIdentifiers(), allMemberClusterNames, nil, resourceSnapshotIndex1st) |
There was a problem hiding this comment.
was this not working? confused because you didn't change any logic but changed the test
Or are they the same just simplify because it rolls out to all 3 clusters anyway?
There was a problem hiding this comment.
- crpStatusUpdatedActual is used to check CRP status when rolloutStrategy is rollingUpdate handled by rollout controller
- crpStatusWithExternalStrategyActual is used to check CRP status when rolloutStrategy is external handled by the updateRun controller
These tests check if transition between the two works and in some cases like this they can be used interchangeably since the other rolloutStrategy has not started immediately once placement spec is updated
There was a problem hiding this comment.
yes, they should be the same.
|
|
||
| It("Should update rp status as completed with new snapshot", func() { | ||
| rpStatusUpdatedActual := rpStatusWithExternalStrategyActual(appConfigMapIdentifiers(), resourceSnapshotIndex2nd, true, allMemberClusterNames, | ||
| []string{resourceSnapshotIndex2nd, resourceSnapshotIndex2nd, resourceSnapshotIndex2nd}, []bool{true, true, true}, nil, nil) |
There was a problem hiding this comment.
Does this work?
rpStatusUpdatedActual := rpStatusUpdatedActual(appConfigMapIdentifiers(), allMemberClusterNames, nil, resourceSnapshotIndex2nd)There was a problem hiding this comment.
yes, should work too. Basically when the clusters are all updated with the same resourceSnapshotIndex, rpStatusWithExternalStrategyActual and rpStatusUpdatedActual should generate same wanted rpStatus.
|
|
||
| It("Should update rp status as completed", func() { | ||
| rpStatusUpdatedActual := rpStatusWithExternalStrategyActual(appConfigMapIdentifiers(), resourceSnapshotIndex1st, true, allMemberClusterNames, | ||
| []string{resourceSnapshotIndex1st, resourceSnapshotIndex1st, resourceSnapshotIndex1st}, []bool{true, true, true}, nil, nil) |
There was a problem hiding this comment.
Does this work?
rpStatusUpdatedActual := rpStatusUpdatedActual(appConfigMapIdentifiers(), allMemberClusterNames, nil, resourceSnapshotIndex1st)| validateLatestResourceSnapshot(rpName, testNamespace, resourceSnapshotIndex2nd) | ||
|
|
||
| // RP status should still show completed with old snapshot. | ||
| rpStatusUpdatedActual := rpStatusWithExternalStrategyActual(appConfigMapIdentifiers(), resourceSnapshotIndex1st, true, allMemberClusterNames, |
There was a problem hiding this comment.
can this be replaced with rpStatusUpdatedActual?
3 participants
Description of your changes
Fixes #
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer