[ECS-Plugin] Implement ECS_ SYNC stage#6559
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>
when recreate is enable, stop the current running tasks (i.e. set desired #tasks of service to 0), then rollout new taskset and set the number of desired #tasks back to original value Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
|
There some works still needed to do before merging, I 've just created this PR so you (@khanhtc1202) can have a quick glance and give early feedbacks ^ ^ To make it easy when reviewing PR, you can refer to these resources: Some changes have been made (different from v0)
func (c *client) ListTags(ctx context.Context, resourceArn string) ([]types.Tag, error) {
input := &ecs.ListTagsForResourceInput{
ResourceArn: aws.String(resourceArn),
}
output, err := c.ecsClient.ListTagsForResource(ctx, input)
if err != nil {
return nil, err
}
tags := make([]types.Tag, 0, len(output.Tags))
for _, t := range output.Tags {
tags = append(tags, types.Tag{
Key: t.Key,
Value: t.Value,
})
}
return tags, nil
}TODO (in works):
|
|
About the testing part, I've already made something like this to allow real integration test. But this is not safe due to potential waste if not handle cleaning up resources properly after test, maybe mocking will be better here var (
clusterARN = os.Getenv("ECS_CLUSTER_ARN")
subnetIDsStr = os.Getenv("SUBNET_IDS")
sgIDsStr = os.Getenv("SECURITY_GROUP_IDS")
subnetIDs []string
sgIDs []string
)
if clusterARN == "" {
t.Skip("ECS_CLUSTER_ARN is not set, skipping ECS sync stage test")
}
if subnetIDsStr == "" {
t.Skip("SUBNET_IDS is not set, skipping ECS sync stage test")
}
for _, id := range strings.Split(subnetIDsStr, ",") {
subnetIDs = append(subnetIDs, strings.TrimSpace(id))
}
if len(subnetIDs) == 0 {
t.Skip("SUBNET_IDS is empty, skipping ECS sync stage test")
}
if sgIDsStr == "" {
t.Skip("SECURITY_GROUP_IDS is not set, skipping ECS sync stage test")
} |
|
For DetermineVersion function, I think you can ignore it in this PR, we can implement it by a separated PR |
|
Also, your commits had shown as |
| // Add PipeCD-managed tags | ||
| serviceDef.Tags = append(serviceDef.Tags, | ||
| provider.MakeTags(map[string]string{ | ||
| provider.LabelManagedBy: provider.ManagedByECSPlugin, | ||
| provider.LabelPiped: input.Request.Deployment.PipedID, | ||
| provider.LabelApplication: input.Request.Deployment.ApplicationID, | ||
| provider.LabelCommitHash: input.Request.TargetDeploymentSource.CommitHash, | ||
| })..., | ||
| ) |
There was a problem hiding this comment.
How about to move in inside LoadServiceDefinition function? Since we may need to do the same when implement rollout stage.
There was a problem hiding this comment.
I think LoadServiecDefinition purpose is parsing manifest into types.Service, adding tags here I just feel not the right place to do that
There was a problem hiding this comment.
IMO, adding tags explicitly in execute stage function will avoid confusion for the maintain work in the future
Also, LoadServiceDefinition current input consists of 2 params: appDir, serviceDefinition string, if moving adding tags to this function, we need to add pipeId, applicationId, commitHash, which will make the function bloat and drift away from its function
There was a problem hiding this comment.
Okay, let re-check the service tags value of service object fetched from API when we update service. If it contains the tags as well, we can safely move this tag assign logic to load service definition, ensure service object in data flow will always have the tags. 👍
For now, let's keep the implementation as your 👍
There was a problem hiding this comment.
The service returned from UpdateService API doesn't contain tags but all the stage use applyServiceDefinition and inside that function the tags are re assigned
// Re-assign tags to service object because UpdateService API doesn't return tags.
service.Tags = serviceDefinition.TagsAnd my bad, the v0 loadServiceDefinition already add the tags inside
func loadServiceDefinition(in *executor.Input, serviceDefinitionFile string, ds *deploysource.DeploySource) (types.Service, bool) {
in.LogPersister.Infof("Loading service manifest at commit %s", ds.Revision)
serviceDefinition, err := provider.LoadServiceDefinition(ds.AppDir, serviceDefinitionFile)
if err != nil {
in.LogPersister.Errorf("Failed to load ECS service definition (%v)", err)
return types.Service{}, false
}
serviceDefinition.Tags = append(
serviceDefinition.Tags,
provider.MakeTags(map[string]string{
provider.LabelManagedBy: provider.ManagedByPiped,
provider.LabelPiped: in.PipedConfig.PipedID,
provider.LabelApplication: in.Deployment.ApplicationId,
provider.LabelCommitHash: in.Deployment.CommitHash(),
})...,
)
in.LogPersister.Infof("Successfully loaded the ECS service definition at commit %s", ds.Revision)
return serviceDefinition, true
}Update:
// LoadServiceDefinition returns Service object from a given service definition file.
func LoadServiceDefinition(appDir, serviceDefinition string, input *sdk.ExecuteStageInput[config.ECSApplicationSpec]) (types.Service, error) {
path := filepath.Join(appDir, serviceDefinition)
service, err := loadServiceDefinition(path)
if err != nil {
return types.Service{}, err
}
// Add PipeCD-managed tags
service.Tags = append(service.Tags,
MakeTags(map[string]string{
LabelManagedBy: ManagedByECSPlugin,
LabelPiped: input.Request.Deployment.PipedID,
LabelApplication: input.Request.Deployment.ApplicationID,
LabelCommitHash: input.Request.TargetDeploymentSource.CommitHash,
})...,
)
return service, nil
}There was a problem hiding this comment.
Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
5f46f2e to
7b35462
Compare
Signed-off-by: Hoang Ngo <adlehoang118@gmail.com>
What this PR does:
Implementing
ECS_SYNCstage. The quick sync and sync option will use this stage by defaultWhy we need it:
Which issue(s) this PR fixes: Part of #6443
Fixes #
Does this PR introduce a user-facing change?: