[ECS-Plugin] Document for user#6712
Conversation
Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
…on file" Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
7ca28b5 to
c93da0a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6712 +/- ##
==========================================
+ Coverage 29.48% 37.92% +8.44%
==========================================
Files 593 61 -532
Lines 63440 5331 -58109
==========================================
- Hits 18706 2022 -16684
+ Misses 43289 3193 -40096
+ Partials 1445 116 -1329
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
eeshaanSA
left a comment
There was a problem hiding this comment.
Hello @armistcxy, is there no way to split this into multiple PRs? This would ease the reviewers, and eventually result in a faster merge.
|
Hi @eeshaanSA, really sorry for this big PR. I think I can't split this into multiple PRs. You can review only the section you think important, I have noted the outline above Also use |
For this, I will have to checkout your PR or serve it on GitHub Codespaces (We have access to CoPilot Enterprise as Maintainers). Please give me sometime, I will try reviewing it. |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
rahulshendre
left a comment
There was a problem hiding this comment.
apart from the nits, overall looks good to me @armistcxy
also SDK struct names on lines 94/121 need user-facing rewrites, also ECSTargetGroups and ECSTargetGroup tables have 3 columns (Field, Type, Description) while every other reference table has 4 (Field, Type, Default, Description). we should add a default column to both same for ECSDeployTargetConfig which is also missing Default.
| - You want a simple, immediate rollout with no traffic splitting. | ||
| - The environment is non-critical (e.g. dev, staging) where gradual rollout adds no value. | ||
|
|
||
| > Standalone tasks always use quick sync. Pipeline sync is not supported for standalone tasks. |
There was a problem hiding this comment.
this is repeated on the line 200 as well. We should keep just the line 200
| - You need manual approval gates before full rollout. | ||
| - You want automated analysis (metrics, logs) to validate the new version before promoting it. | ||
|
|
||
| ## Quick sync |
There was a problem hiding this comment.
this should be ### as they're subsections of - choosing a deployment strategy
There was a problem hiding this comment.
Actually, the content of "Choosing a deployment strategy" focuses on deciding between quick sync and pipeline sync, it explains which option is more appropriate for specific use cases. And the detailed configuration and setup instructions for each strategy are covered in separate ## sections below
There was a problem hiding this comment.
makes sense, please ignore this one
|
|
||
| ECS deployments involve three distinct IAM roles. Confusing them is a common source of permission errors. | ||
|
|
||
| **Deployment role**: the role assumed by Piped when calling AWS APIs to deploy your application. Configured via `roleARN` in the deploy target config, or resolved from the default credential chain. This role needs: |
There was a problem hiding this comment.
also in back ticks please. All other occurrences as well. piped
|
|
||
| ## Application live state | ||
|
|
||
| The ECS plugin continuously monitors your application's live state and displays it on the PipeCD UI. Piped periodically polls AWS and compares the running state against the commit hash declared in Git. |
There was a problem hiding this comment.
actually there're many places in the document also put "Piped", maybe if we need consistency we should change to pipedv1, do you think it's ok ?
There was a problem hiding this comment.
just piped in backticks, consistent with what @eeshaanSA said above
|
|
||
| The comparison checks each container by name. If the image name differs, the full change is reported; if only the tag or digest differs, the version change is reported. All detected changes appear in the deployment summary in the UI. | ||
|
|
||
| > Non-image changes (environment variables, CPU/memory, etc.) do not affect strategy selection. If you update only those fields, the plugin will choose quick sync even when a pipeline is configured. |
There was a problem hiding this comment.
this should have Note: prefix, for consistency
|
|
||
| By default, when no `pipeline` is specified in the application configuration, PipeCD triggers a **quick sync** for any merged pull request. Quick sync registers the new task definition, creates/updates the ECS service, promotes a new primary task set, waits for stability, and removes old task sets. All traffic is switched to the new version immediately. | ||
|
|
||
| > Standalone tasks always use quick sync. Pipeline sync is not supported for standalone tasks. |
There was a problem hiding this comment.
this too the note prefix
There was a problem hiding this comment.
Nice catch, I will fix it
|
|
||
| **EXTERNAL controller** (required by this plugin for canary/blue-green): | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add ```text to the resource tree code block, without it, renderers may mangle the └── characters.
There was a problem hiding this comment.
as far as I remember when I rendered there's nothing wrong with it. Let me check when I complete all the fixes above
| |---|---| | ||
| | Task definition diff | Unified diff between the running `taskdef` file and the target `taskdef` file | | ||
| | Service definition diff | Unified diff between the running `servicedef` file and the target `servicedef` file (only shown when `serviceDefinitionFile` is set) | | ||
| | Summary | A one-line summary: `task definition changed`, `service definition changed`, or `No changes were detected` | |
There was a problem hiding this comment.
No changes were detected has a trailing period while every other entry doesn't, we should remove it
There was a problem hiding this comment.
Hmm, I don't see a trailing period in "No changes were detected". Could you point me to where you're seeing it? The only inconsistency I notice is the capitalization though ^^
There was a problem hiding this comment.
my bad, you can proceed with the capitalization fixes :D
There was a problem hiding this comment.
Will later fix with chore PR
|
Thanks @rahulshendre for your review, I will take a look |
eeshaanSA
left a comment
There was a problem hiding this comment.
Apart from the nits, everything largely looks good.
|
|
||
| ECS deployments involve three distinct IAM roles. Confusing them is a common source of permission errors. | ||
|
|
||
| **Deployment role**: the role assumed by Piped when calling AWS APIs to deploy your application. Configured via `roleARN` in the deploy target config, or resolved from the default credential chain. This role needs: |
There was a problem hiding this comment.
also in back ticks please. All other occurrences as well. piped
|
|
||
| ### Blue/Green deployment | ||
|
|
||
| Deploys the new version (green) at full scale alongside the old version (blue), then cuts over all traffic atomically after approval. Requires **two ALB target groups** (primary and canary) both attached to the same listener. |
|
|
||
| The application config format changes in two ways: the `kind` field and the location of ECS-specific fields. | ||
|
|
||
| **v0 app config:** |
There was a problem hiding this comment.
better to say: "v0 application.config.yaml file"
or
"application configuration file"
| - name: ECS_CANARY_CLEAN | ||
| ``` | ||
|
|
||
| **v1 app config:** |
|
|
||
| ## Changes from v0 | ||
|
|
||
| This section describes behavioral differences between the legacy ECS provider in PipeCD v0 and the ECS plugin in PipeCD v1. If you are migrating from v0, review these changes before deploying. |
There was a problem hiding this comment.
How about putting everything related to v0 in one section? Like in one the sections above? You can make multiple subsections there? thoughts?
There was a problem hiding this comment.
Hard to make decision here because I'm developing a feature supporting new controller type. So let just keep this document version flat and I will need your consulting later ^^
|
|
||
| This section describes behavioral differences between the legacy ECS provider in PipeCD v0 and the ECS plugin in PipeCD v1. If you are migrating from v0, review these changes before deploying. | ||
|
|
||
| ### ECS_PRIMARY_ROLLOUT: only the old PRIMARY task set is removed |
There was a problem hiding this comment.
better to have a cleaner title, and say the rest in in the body of the section
There was a problem hiding this comment.
Overall looks good. @armistcxy
But, some sections can be actually separated into different files, for example, the config reference, migration.
of course, it is just a readability thing, I know this is urgent, so we can have it merged, and then later create a section for each plugin, and multiple files under it.
Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
557ce62 to
4594ad0
Compare
|
@eeshaanSA @rahulshendre I think it's all good now, some changes will be postponed to the next version of ECS plugin |

What this PR does: Adds user documentation for the ECS plugin (
docs-v1.0.x) covering:ECS_PRIMARY_ROLLOUT,ECS_CANARY_ROLLOUT,ECS_TRAFFIC_ROUTING,ECS_CANARY_CLEAN,ECS_ROLLBACKWhy we need it:
Which issue(s) this PR fixes:
Fixes #6443
Does this PR introduce a user-facing change?: