feat: Start/Stop API Implementation #346
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
britaniar
force-pushed
the
startStopImplementation
branch
from
9884da7 to
07c7ac6
Compare
Signed-off-by: Britania Rodriguez Reyes <[email protected]>
britaniar
force-pushed
the
startStopImplementation
branch
from
07c7ac6 to
6a68b64
Compare
Signed-off-by: Britania Rodriguez Reyes <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR implements the Start/Stop API for update runs, introducing state-based control over update run execution. The changes enable users to create update runs in a NotStarted state, transition them to Started to begin execution, Stopped to pause, and Abandoned to terminate. Key changes include:
- Updated the updateRun controller to use
Statefrom the Spec to determine execution flow - Modified conditional checks to disregard generation for immutable fields (only State changes)
- Refactored
executeUpdatingStageinto smaller helper methods to reduce complexity - Added comprehensive integration and E2E tests for state transitions
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/controllers/updaterun/controller.go | Added state-based execution control with early checks for Abandoned/Stopped states; modified initialization logic to ignore generation changes |
| pkg/controllers/updaterun/execution.go | Refactored executeUpdatingStage into smaller helper methods; removed generation checks from condition evaluations |
| pkg/controllers/updaterun/validation.go | Updated condition checks to be generation-independent for succeeded/failed states |
| pkg/utils/condition/reason.go | Added new condition reasons: UpdateRunPausedReason and UpdateRunAbandonedReason |
| test/e2e/staged_updaterun_test.go | Updated all createStagedUpdateRunSucceed calls to include state parameter; added comprehensive E2E test for state transitions |
| test/e2e/cluster_staged_updaterun_test.go | Updated all createClusterStagedUpdateRunSucceed calls to include state parameter; added E2E test for cluster-scoped state transitions |
| test/e2e/actuals_test.go | Added helper functions for validating NotStarted, Stopped, and Abandoned states; refactored status building logic |
| pkg/controllers/updaterun/*_integration_test.go | Updated test helpers to use reason strings; added integration tests for NotStarted/Stopped/Abandoned states |
|
|
||
| By("Validating crp status as member-cluster-2 updated only") | ||
| rpStatusUpdatedActual := rpStatusWithExternalStrategyActual(nil, "", false, allMemberClusterNames, []string{"", resourceSnapshotIndex1st, ""}, []bool{false, true, false}, nil, nil) | ||
| Eventually(rpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update RP %s status as expected", crpName) |
There was a problem hiding this comment.
The variable name crpName is used in the error message, but this test is for namespaced resource placements which use rpName. The error message should use rpName instead to match the test context and be consistent with other assertions in this test (see lines 1527, 1541).
| Eventually(rpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update RP %s status as expected", crpName) | |
| Eventually(rpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update RP %s status as expected", rpName) |
| }) | ||
| }) | ||
|
|
||
| //TODO(britaniar): Add more e2e tests for updateRun Start/Stop Implementation |
There was a problem hiding this comment.
The TODO comment lacks a reference to a tracking issue. Consider adding an issue link or ticket number to track this work, following the pattern used elsewhere in the codebase.
| //TODO(britaniar): Add more e2e tests for updateRun Start/Stop Implementation | |
| //TODO(britaniar): Add more e2e tests for updateRun Start/Stop Implementation (see issue #1234) |
| }) | ||
| }) | ||
|
|
||
| //TODO(britaniar): Add more e2e tests for updateRun Start/Stop Implementation |
There was a problem hiding this comment.
The TODO comment lacks a reference to a tracking issue. Consider adding an issue link or ticket number to track this work, following the pattern used elsewhere in the codebase.
| //TODO(britaniar): Add more e2e tests for updateRun Start/Stop Implementation | |
| //TODO(britaniar): Add more e2e tests for updateRun Start/Stop Implementation. See issue #1234 |
| Eventually(surSucceededActual, updateRunEventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to validate updateRun %s succeeded", updateRunNames[0]) | ||
| }) | ||
|
|
||
| It("Should stop update run when updated to Abandoned state", func() { |
There was a problem hiding this comment.
Misleading test description. The test says "Should stop update run" but it actually abandons the update run. Consider changing to: "Should abandon update run when updated to Abandoned state"
| It("Should stop update run when updated to Abandoned state", func() { | |
| It("Should abandon update run when updated to Abandoned state", func() { |
| Eventually(csurSucceededActual, updateRunEventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to validate updateRun %s succeeded", updateRunNames[0]) | ||
| }) | ||
|
|
||
| It("Should stop update run when updated to Abandoned state", func() { |
There was a problem hiding this comment.
Misleading test description. The test says "Should stop update run" but it actually abandons the update run. Consider changing to: "Should abandon update run when updated to Abandoned state"
| It("Should stop update run when updated to Abandoned state", func() { | |
| It("Should abandon update run when updated to Abandoned state", func() { |
1 participant
Description of your changes
I have:
Update updateRun controller to use State from the Spec to determine whether to start/stop/abandon update run.
Updated conditional checks to disregard generation checks as update run is immutable but the State field.
Refactor
(*Reconciler).executeUpdatingStageto help with cyclomatic complexity (linter failure)Added/update integrations tests and e2e tests.
Run
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer