Added error check for canary promote actions#432
Conversation
|
I think we should also add the error check here for blue green strategy: https://github.com/Azure/k8s-deploy/blob/cc1e193d235831603275f9b90189a68ffbb4af4f/src/strategyHelpers/blueGreen/blueGreenHelper.ts#L268C1-L282C2 |
…21/k8s-deploy into bugfix/canary_promote_action
|
Just to note here, I reverted back to the commit that had some integration errors. The issue is at the https://github.com/Azure/k8s-deploy/pull/432/files#diff-bb3e08b6d71c82cd46d12233b7c37fdb13932d2e9d774bb80eb3b3e646520ca4R281, an empty manifestFiles was being passed. This was not handled well and wasn't detected till the checkForErrors was introduced. PR #425 contains this fix and should be merged before this. This should fix the error. Thank you. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds error checking after Kubernetes apply commands in both canary and blue-green workflows and updates related tests and manifests accordingly.
- Introduces a new
basic-test.ymlmanifest and adjusts file search tests to include it. - Inserts
checkForErrorscalls afterkubectl.applyin SMICanary, PodCanary, and BlueGreen helpers. - Enhances tests with shared mock objects and consolidated error scenarios across multiple strategy helpers.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/manifests/basic-test.yml | New basic manifest used in unit tests |
| src/utilities/fileUtils.test.ts | Updated expected file list and lengths to include basic-test.yml |
| src/strategyHelpers/canary/smiCanaryHelper.ts | Added checkForErrors after kubectl.apply |
| src/strategyHelpers/canary/podCanaryHelper.ts | Added import and call to checkForErrors after apply |
| src/strategyHelpers/blueGreen/blueGreenHelper.ts | Added checkForErrors after kubectl.apply in deployObjects |
Multiple *.test.ts files across canary and blueGreen dirs |
Refactored tests for shared mocks and added consolidated error checks |
Tatsinnit
left a comment
There was a problem hiding this comment.
❤️ Thanks for kind ping for the review of this PR, I've had another Quick Look, and shared some of my observations. Please get review from @bosesuneha and @davidgamero for better e2e understanding.
This PR ensures that any failed Kubernetes apply operations in canary and blue-green workflows properly abort execution by invoking checkForErrors and includes tests to ensure this works. It also adds a new test manifest and updates related tests to include it.
This PR fixes issue Bug: Canary deployment the promote action doesn't fail even when kubectl apply return error #305