Skip to content

fix: set updaterun progressing condition to false when finished#1103

Merged
jwtty merged 3 commits intoAzure:mainfrom
jwtty:progressing-cond-fix
Apr 3, 2025
Merged

fix: set updaterun progressing condition to false when finished#1103
jwtty merged 3 commits intoAzure:mainfrom
jwtty:progressing-cond-fix

Conversation

@jwtty
Copy link
Contributor

@jwtty jwtty commented Apr 2, 2025

Description of your changes

When updateRun or a stage completes (either succeeds or fails), set progressing condition to false.
Also add missing condition messages.

Fixes #

I have:

  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Special notes for your reviewer

@kaito-pr-agent
Copy link

kaito-pr-agent bot commented Apr 2, 2025

Title

(Describe updated until commit b748903)

fix: set updaterun progressing condition to false when finished


User description

Description of your changes

When updateRun or a stage completes (either succeeds or fails), set progressing condition to false.
Also add missing condition messages.

Fixes #

I have:

  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Special notes for your reviewer


PR Type

Enhancement


Description

  • Added support for setting the progressing condition to false when an update run or stage completes.

  • Updated condition messages for better clarity.

  • Enhanced test cases to cover the new progressing condition logic.


Changes walkthrough 📝

Relevant files
Enhancement
controller.go
Add progressing condition updates                                               

pkg/controllers/updaterun/controller.go

  • Added code to set the progressing condition to false when an update
    run or stage completes.
  • Updated condition messages for better clarity.
  • +15/-0   
    Tests
    controller_integration_test.go
    Update integration tests for progressing conditions           

    pkg/controllers/updaterun/controller_integration_test.go

    • Updated test cases to cover the new progressing condition logic.
    +23/-0   
    Additional files
    execution.go +24/-0   
    execution_integration_test.go +17/-13 
    validation_integration_test.go +4/-3     
    actuals_test.go +11/-16 

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @kaito-pr-agent
    Copy link

    kaito-pr-agent bot commented Apr 2, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit b748903)

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    500 - Partially compliant

    Compliant requirements:

    • Ensure that the progressing condition is set to false when the updateRun or a stage completes.
    • Add missing condition messages.

    Non-compliant requirements:

    • None

    Requires further human verification:

    • None
     Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Bug

    The new code sets the progressing condition to false without checking if the operation was successful or not. This could lead to incorrect status updates.

    meta.SetStatusCondition(&updateRun.Status.Conditions, metav1.Condition{
    	Type:               string(placementv1beta1.StagedUpdateRunConditionProgressing),
    	Status:             metav1.ConditionFalse,
    	ObservedGeneration: updateRun.Generation,
    	Reason:             condition.UpdateRunSucceededReason,
    	Message:            "All stages are completed",
    })
    Potential Bug

    The new code sets the progressing condition to false without checking if the operation was successful or not. This could lead to incorrect status updates.

    meta.SetStatusCondition(&updateRun.Status.Conditions, metav1.Condition{
    	Type:               string(placementv1beta1.StagedUpdateRunConditionProgressing),
    	Status:             metav1.ConditionFalse,
    	ObservedGeneration: updateRun.Generation,
    	Reason:             condition.UpdateRunFailedReason,
    	Message:            "The stages are aborted due to a non-recoverable error",
    })

    @kaito-pr-agent
    Copy link

    kaito-pr-agent bot commented Apr 2, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Set Progressing condition before Succeeded condition

    Ensure the Progressing condition is set before setting the Succeeded condition.

    pkg/controllers/updaterun/controller.go [188-194]

     meta.SetStatusCondition(&updateRun.Status.Conditions, metav1.Condition{
         Type:               string(placementv1beta1.StagedUpdateRunConditionProgressing),
         Status:             metav1.ConditionFalse,
         ObservedGeneration: updateRun.Generation,
         Reason:             condition.UpdateRunSucceededReason,
         Message:            "All stages are completed",
     })
    +meta.SetStatusCondition(&updateRun.Status.Conditions, metav1.Condition{
    +    Type:               string(placementv1beta1.StagedUpdateRunConditionSucceeded),
    +    Status:             metav1.ConditionTrue,
    +    ObservedGeneration: updateRun.Generation,
    +    Reason:             condition.UpdateRunSucceededReason,
    +    Message:            "ClusterStagedUpdateRun completed successfully",
    +})
    Suggestion importance[1-10]: 7

    __

    Why: Ensuring the Progressing condition is set before the Succeeded condition helps maintain logical consistency in the status updates.

    Medium
    Set Progressing condition before Waiting condition

    Ensure the Progressing condition is set before setting the Waiting condition.

    pkg/controllers/updaterun/controller_integration_test.go [559-564]

    -wantStatus.StagesStatus[0].Conditions[0] = generateFalseCondition(updateRun, placementv1beta1.StageUpdatingConditionProgressing) // The progressing condition now becomes false with waiting reason.
    +wantStatus.StagesStatus[0].Conditions[0] = generateFalseProgressingCondition(updateRun, placementv1beta1.StageUpdatingConditionProgressing, false) // The progressing condition now becomes false with waiting reason.
    Suggestion importance[1-10]: 7

    __

    Why: Ensuring the Progressing condition is set before the Waiting condition maintains logical consistency in the test conditions.

    Medium
    Set Progressing condition before Failed condition

    Ensure the Progressing condition is set before setting the Failed condition.

    pkg/controllers/updaterun/execution.go [496-502]

    +meta.SetStatusCondition(&stageUpdatingStatus.Conditions, metav1.Condition{
    +    Type:               string(placementv1beta1.StageUpdatingConditionProgressing),
    +    Status:             metav1.ConditionFalse,
    +    ObservedGeneration: generation,
    +    Reason:             condition.StageUpdatingFailedReason,
    +    Message:            "Stage update aborted due to a non-recoverable error",
    +})
     
    -
    Suggestion importance[1-10]: 7

    __

    Why: Ensuring the Progressing condition is set before the Failed condition maintains logical consistency in the status updates.

    Medium

    @jwtty jwtty force-pushed the progressing-cond-fix branch from 4f76653 to af18c9c Compare April 2, 2025 03:35
    Co-authored-by: Zhiying Lin <54013513+zhiying-lin@users.noreply.github.com>
    @kaito-pr-agent
    Copy link

    kaito-pr-agent bot commented Apr 2, 2025

    Persistent review updated to latest commit 20e7d0c

    Co-authored-by: Zhiying Lin <54013513+zhiying-lin@users.noreply.github.com>
    @kaito-pr-agent
    Copy link

    kaito-pr-agent bot commented Apr 2, 2025

    Persistent review updated to latest commit b748903

    @jwtty jwtty merged commit b7f278a into Azure:main Apr 3, 2025
    16 checks passed
    @jwtty jwtty deleted the progressing-cond-fix branch April 3, 2025 06:05
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    None yet

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants