Skip to content

Refactor upgrade logic to skip pre upgrade job in case of patch revisions#138

Merged
anshumanks merged 9 commits intodevelopfrom
controller_skip_preupgrade
Feb 14, 2025
Merged

Refactor upgrade logic to skip pre upgrade job in case of patch revisions#138
anshumanks merged 9 commits intodevelopfrom
controller_skip_preupgrade

Conversation

@anshumanks
Copy link
Copy Markdown
Contributor

@anshumanks anshumanks commented Feb 12, 2025

Refactor upgrade logic to skip pre upgrade job in case of patch revisions

Description

This change prevents pre upgrade job from stopping pipelines in case of patch revisions.

Code change

  • Modified version_update.go, constants.go

Unit Tests

  • Modified version_update_test.go
  • Modified old tests to incorporate new return value from compareVersions function (-2 for patch revisions)
  • Added a new test to check for patch revisions

Tested

In case of patch revisions:
Screenshot 2025-02-13 at 7 39 57 AM

Patch revision with skip pre upgrade flag = false:
Screenshot 2025-02-13 at 7 58 26 AM
Screenshot 2025-02-13 at 8 53 20 AM

In other cases:
Screenshot 2025-02-13 at 6 59 30 AM

log.Printf("Version update: patch revision detected, skipping pre-upgrade and post-upgrade jobs.")
// Mark pre and post upgrade jobs as succeeded so they don't get triggered when reconciling
// patchRevision will become false after reconciliation
setCondition(master, updateStatus.PreUpgradeSucceeded)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to mark them as succeeded if you haven't run it.

In case of patch revision, the logic would probably the same as in the case of versionComparison == 0.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was the initial plan, but if I add something like if !isConditionTrue(master, updateStatus.Pre/PostUpgradeSucceeded) && !skipPreUpgrade, this will skip pre upgrade, and change the image. Then versionComparison will become 0.
If we incorporate 0 along with -4 in skipPreUpgrade ( versionComparison == -4 || versionComparison == 0 ), for minor/major changes the post upgrade job will get skipped.

Possible fix: Skip pre upgrade in case of 0 and -4, and modify post upgrade condition as
!isConditionTrue(master, updateStatus.PostUpgradeSucceeded) && isConditionTrue(master, updateStatus.PreUpgradeSucceeded)

case -1:
// Upgrade case

if versionComparison < 0 { // Upgrade case
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: please add comments above the code instead of inline. Comment sentences should end with a period (.)

Fix t/o

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