Skip to content

fix: use min requested index for ecs rollback stage - #7222

Merged
armistcxy merged 5 commits into
masterfrom
fix/ecs-build-pipeline
Aug 23, 2026
Merged

fix: use min requested index for ecs rollback stage#7222
armistcxy merged 5 commits into
masterfrom
fix/ecs-build-pipeline

Conversation

@armistcxy

Copy link
Copy Markdown
Contributor

What this PR does:

  • Fixes buildPipelineStages in ECS plugin to reuse the smallest requested index for the appended rollback stage
  • Adds unit tests covering the pipeline/quick-sync stage builders

Why we need it: sdk.PipelineStage.Index requires each returned index to be one of the indexes from the request. The previous implementation returned an invented index for ECS_ROLLBACK, so piped's validateStageIndexes rejected the plugin response

Signed-off-by: armistcxy <adlehoang118@gmail.com>
@netlify

netlify Bot commented Aug 23, 2026

Copy link
Copy Markdown

Deploy Preview for pipecd-site canceled.

Name Link
🔨 Latest commit f3ff3fb
🔍 Latest deploy log https://app.netlify.com/projects/pipecd-site/deploys/6a8a8c431e3cdc0008503706

@codecov

codecov Bot commented Aug 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 52.94118% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 29.74%. Comparing base (8ed5a0e) to head (f3ff3fb).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
pkg/app/pipedv1/plugin/ecs/deployment/plugin.go 0.00% 5 Missing ⚠️
pkg/app/pipedv1/plugin/ecs/deployment/pipeline.go 75.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7222      +/-   ##
==========================================
+ Coverage   29.68%   29.74%   +0.05%     
==========================================
  Files         601      601              
  Lines       64396    64408      +12     
==========================================
+ Hits        19116    19158      +42     
+ Misses      43789    43758      -31     
- Partials     1491     1492       +1     
Flag Coverage Δ
. 23.59% <ø> (ø)
.-pkg-app-pipedv1-plugin-analysis 32.16% <ø> (ø)
.-pkg-app-pipedv1-plugin-ecs 33.64% <52.94%> (+1.90%) ⬆️
.-pkg-app-pipedv1-plugin-kubernetes 58.39% <ø> (ø)
.-pkg-app-pipedv1-plugin-kubernetes_multicluster 59.62% <ø> (ø)
.-pkg-app-pipedv1-plugin-scriptrun 54.83% <ø> (ø)
.-pkg-app-pipedv1-plugin-terraform 38.46% <ø> (ø)
.-pkg-app-pipedv1-plugin-wait 33.04% <ø> (ø)
.-pkg-app-pipedv1-plugin-waitapproval 52.71% <ø> (ø)
.-pkg-plugin-sdk 49.85% <ø> (ø)
.-tool-actions-gh-release 19.23% <ø> (ø)
.-tool-actions-plan-preview 25.51% <ø> (ø)
.-tool-codegen-protoc-gen-auth 0.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Warashi Warashi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We need this logic in all of the deployment plugins, so it's good to port it to the SDK?

Comment thread pkg/app/pipedv1/plugin/ecs/deployment/pipeline.go

Copilot AI left a comment

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.

Pull request overview

This PR fixes the ECS pipedv1 plugin’s buildPipelineStages to ensure the appended ECS_ROLLBACK stage uses a request-provided stage index (specifically the minimum requested index), satisfying piped’s validateStageIndexes contract and preventing plugin responses from being rejected.

Changes:

  • Update ECS pipeline stage builder to assign the rollback stage the minimum requested stage index (instead of an invented index).
  • Add unit tests for ECS pipeline stage building, rollback index contract, and quick-sync stage building.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
pkg/app/pipedv1/plugin/ecs/deployment/pipeline.go Reuses the minimum requested index for ECS_ROLLBACK to satisfy piped’s stage-index validation.
pkg/app/pipedv1/plugin/ecs/deployment/pipeline_test.go Adds unit tests for pipeline/quick-sync builders and a contract check ensuring returned indexes exist in the request.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread pkg/app/pipedv1/plugin/ecs/deployment/pipeline.go
}
}

// replica of piped's controller.validateStageIndexes (unexported there), kept in sync manually.
Signed-off-by: armistcxy <adlehoang118@gmail.com>
Signed-off-by: armistcxy <adlehoang118@gmail.com>
@armistcxy
armistcxy force-pushed the fix/ecs-build-pipeline branch from 14f021d to 74705e8 Compare August 23, 2026 05:51
Signed-off-by: armistcxy <adlehoang118@gmail.com>
@armistcxy
armistcxy force-pushed the fix/ecs-build-pipeline branch from ab89331 to 3f56b54 Compare August 23, 2026 05:56
@armistcxy

armistcxy commented Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

We need this logic in all of the deployment plugins, so it's good to port it to the SDK?

At this point I don't have my answer yet, but I think this part is actually repetitive among plugins. So let me investigate first

@Warashi Warashi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can open another PR to make this part of the SDK, so I approve this as-is.

@armistcxy
armistcxy merged commit 69545ec into master Aug 23, 2026
67 checks passed
@armistcxy
armistcxy deleted the fix/ecs-build-pipeline branch August 23, 2026 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants