-
Notifications
You must be signed in to change notification settings - Fork 20
feat: update run abandon implementation #360
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
base: main
Are you sure you want to change the base?
Conversation
pkg/controllers/updaterun/abandon.go
Outdated
| ) | ||
|
|
||
| // abandon handles the abandoning of the update run. | ||
| func (r *Reconciler) abandon( |
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.
taking suggestions on naming this function and the following function as it can used by abandon and pause. Same goes for the file name.
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.
Thinking pause or halt would be best?
Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>
7fd0f54 to
8c18a61
Compare
Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>
Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>
| Reason: condition.UpdateRunAbandonedReason, | ||
| Message: "The update run has been abandoned", | ||
| }) | ||
| meta.SetStatusCondition(&updateRunStatus.Conditions, metav1.Condition{ |
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.
What happens if an updateRun is already Succeeded, and then user tries to abandon the updateRun ?
| var stuckClusterNames []string | ||
| var clusterUpdateErrors []error | ||
| // Go through each cluster in the stage and check if it's updating/succeeded/failed/not started. | ||
| for i := 0; i < len(updatingStageStatus.Clusters) && clusterUpdatingCount < maxConcurrency; i++ { |
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.
We don't need to care about maxConcurrency here, since we are not updating any cluster just waiting for them to complete
Description of your changes
I have:
Update the abandon state variable name to match description.
Update the controller to take into account when to initialize, execute, abandon.
Created a new file to include all abandon related functions in one place.
Updated integration tests.
Run
make reviewableto ensure this PR is ready for review.How has this code been tested
Will add e2e tests later.
Special notes for your reviewer