Reimplement ECS Sync with EXTERNAL deployment controller #2007
Conversation
|
Code coverage for golang is |
khanhtc1202
changed the title
|
Code coverage for golang is |
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get primary task set of service %s: %w", *service.ServiceName, err) | ||
| } | ||
| taskSets := output.Services[0].TaskSets |
There was a problem hiding this comment.
Check to ensure the Services slice is not empty.
There was a problem hiding this comment.
Addressed on 4b5761b 🙏
| return &taskSet, nil | ||
| } | ||
| } | ||
| return nil, nil |
There was a problem hiding this comment.
Not found should be an error as we always do.
There was a problem hiding this comment.
Addressed on 8808feb 🙏
pkg/app/piped/executor/ecs/ecs.go
Outdated
| prevPrimaryTaskSet, err := client.GetPrimaryTaskSet(ctx, *service) | ||
| if err != nil { | ||
| in.LogPersister.Errorf("Failed to create ECS task set %s: %v", *serviceDefinition.ServiceName, err) | ||
| } |
There was a problem hiding this comment.
nit: I feel it more readable to move this part to L162 as related things get close together.
There was a problem hiding this comment.
In that case, we will get the newly promoted (to primary) task set since we called UpdateServicePrimaryTaskSet at L158 👀
There was a problem hiding this comment.
we will 👍
makes sense, never mind
|
LGTM except for the not found error part mentioned by nghialv |
|
Code coverage for golang is |
|
Code coverage for golang is |
|
The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use Details1. Support update other properties of service.This was created by todo plugin since "TODO:" was found in 8808feb when #2007 was merged. cc: @khanhtc1202.2. Support tags for registering task definition.This was created by todo plugin since "TODO:" was found in 8808feb when #2007 was merged. cc: @khanhtc1202.3. Find better way to get those 2 values instead of set it via service def.This was created by todo plugin since "TODO:" was found in 8808feb when #2007 was merged. cc: @khanhtc1202. |
|
/approve |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
Which issue(s) this PR fixes:
Related to #1568
Does this PR introduce a user-facing change?: