Support QuickSync for ECS without ELB or target groups #4685

t-kikuc · 2023-11-22T09:40:46Z

What this PR does / why we need it:
This PR supports QuickSync for ECS using Service Discovery, not ELB and target groups.
I will support canary and blue/green for Service Discovery later.

Which issue(s) this PR fixes:

Part of #4616

Does this PR introduce a user-facing change?:

How are users affected by this change: not affected including configs.
Is this breaking change: no
How to migrate (if breaking change): no

codecov · 2023-11-22T09:44:37Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (8bad596) 30.82% compared to head (8159ba7) 30.83%.

Files	Patch %	Lines
pkg/config/application_ecs.go	83.33%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4685   +/-   ##
=======================================
  Coverage   30.82%   30.83%           
=======================================
  Files         221      221           
  Lines       25935    25947   +12     
=======================================
+ Hits         7995     8001    +6     
- Misses      17289    17295    +6     
  Partials      651      651

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

pkg/config/application_ecs.go

khanhtc1202 · 2023-11-22T10:01:42Z

pkg/config/testdata/application/ecs-app-service-discovery.yaml

+  input:
+    serviceDefinitionFile: /path/to/servicedef.yaml
+    taskDefinitionFile: /path/to/taskdef.yaml
+    accessType: SERVICE_DISCOVERY


This constant is defined by us or by AWS lib? 🤔

"This constant" means 'SERVICE_DISCOVERY'?

If so, it's defined by us, not by AWS.

And other names would be added in the future, like 'APP_MESH'

Then we should define these values as constant of ECS platform provider and have validate in config package

thanks, I got it. I'll fix it.

@khanhtc1202
I did it. Could you check again?
919cc3c

khanhtc1202 · 2023-11-23T10:50:26Z

/review

github-actions · 2023-11-23T10:50:52Z

PR Analysis

Main theme

This PR introduces support for two types of access to ECS services: ELB and service discovery.

PR summary

This PR adds a new field, AccessType, to the ECSDeploymentInput struct in the application_ecs.go file. It also adds validation for the AccessType field and updates the ensureSync function in the deploy.go file to handle the different access types.

Type of PR

Refactoring

PR Feedback:

General suggestions

The changes in this PR look good overall. The addition of support for different access types is a valuable enhancement. The code is well-structured, and the use of constants for the access types improves readability.

Code feedback

pkg/app/piped/executor/ecs/deploy.go

Line 97:
- suggestion: Consider extracting the logic of parsing target groups into a separate function to improve modularity and code readability. [medium]
Line 104:
- suggestion: Since the loadTargetGroups function is only called when ecsInput.IsAccessedViaELB() is true, you can simplify the code by using an if statement instead of an if-else statement. [medium]

pkg/config/application_ecs.go

Line 16:
- suggestion: It would be helpful to add comments explaining the purpose of the constant accessTypeELB. [medium]
Line 17:
- suggestion: Please consider renaming the constant accessTypeServiveDiscovery to accessTypeServiceDiscovery to fix the typo. [medium]
Line 42:
- suggestion: Consider adding a comment describing the purpose of the validateAccessType method. [medium]
Line 47:
- suggestion: Use a switch-case statement instead of an if-else ladder to validate the AccessType field in the ECSDeploymentInput struct. This will improve code readability. [medium]

pkg/config/application_ecs_test.go

Line 70:
- suggestion: Add a test case to cover the scenario where AccessType is set to SERVICE_DISCOVERY. [important]

Security concerns:

No

t-kikuc · 2023-11-23T13:20:28Z

/review

github-actions · 2023-11-23T13:20:57Z

PR Analysis

Main theme

type: Refactoring
description: Adding support for ECS service access type in the application configuration.

PR summary

type: Refactoring
description: This PR introduces changes to support different ways of accessing ECS services by adding an AccessType field to the ECSDeploymentInput struct in the ECS application configuration.

Type of PR

Refactoring

PR Feedback:

General suggestions

The changes look good overall. The addition of the AccessType field enhances the flexibility of ECS deployments. However, there are a few suggestions that could further improve the new code.

Code feedback

File: pkg/app/piped/executor/ecs/deploy.go

L9

The import statement for the github.com/aws/aws-sdk-go-v2/service/ecs/types package is unused and can be removed.

L19-L25

The check for ecsInput.IsAccessedViaELB() can be simplified. Instead of wrapping the subsequent code block with an if condition, you can return model.StageStatus_STAGE_FAILURE directly if !ecsInput.IsAccessedViaELB() is true. This would eliminate the need for the ok variable and reduce the nesting level.

File: pkg/config/application_ecs.go

L18-L27

Consider extracting the constant access type values (ELB and SERVICE_DISCOVERY) into their own const block for better code organization.

L41-L42

The validateAccessType() function can be improved by returning an error message that reflects the invalid AccessType. For example, you can modify the error message to fmt.Errorf("invalid accessType: %s", in.AccessType). This would provide a more specific error message when an invalid AccessType is used.

File: pkg/config/application_ecs_test.go

L71-L77

Consider adding additional test cases to cover the scenarios where accessType is set to SERVICE_DISCOVERY and an invalid AccessType value is used. This would provide broader test coverage for the new AccessType feature.

Security concerns

no

pkg/config/application_ecs.go

khanhtc1202 · 2023-11-27T04:51:20Z

@t-kikuc commented 👍

Signed-off-by: t-kikuc <[email protected]>

t-kikuc · 2023-11-27T06:32:59Z

@khanhtc1202
I removed unrelated commits.

One test/go failed, but it's due to flakiness #4630
And it's passed by a re-run.

pkg/app/piped/executor/ecs/deploy.go

Signed-off-by: t-kikuc <[email protected]>

khanhtc1202

LGTM 🚀

* Support QuickSync for ECS Service Discovery pattern Signed-off-by: t-kikuc <[email protected]> * Update the test expectation of ELB pattern Signed-off-by: t-kikuc <[email protected]> * Add a test of QuickSync of ECS Service Discovery Signed-off-by: t-kikuc <[email protected]> * fix comment Signed-off-by: t-kikuc <[email protected]> * Add a description of Input values Signed-off-by: t-kikuc <[email protected]> * Add consts and validation of accessType Signed-off-by: t-kikuc <[email protected]> * Modify consts to public for using in switch-case Signed-off-by: t-kikuc <[email protected]> * Fix a typo Signed-off-by: t-kikuc <[email protected]> * Refactor validation func Signed-off-by: t-kikuc <[email protected]> * Add a blank line in import Signed-off-by: t-kikuc <[email protected]> --------- Signed-off-by: t-kikuc <[email protected]>

* Support QuickSync for ECS Service Discovery pattern Signed-off-by: t-kikuc <[email protected]> * Update the test expectation of ELB pattern Signed-off-by: t-kikuc <[email protected]> * Add a test of QuickSync of ECS Service Discovery Signed-off-by: t-kikuc <[email protected]> * fix comment Signed-off-by: t-kikuc <[email protected]> * Add a description of Input values Signed-off-by: t-kikuc <[email protected]> * Add consts and validation of accessType Signed-off-by: t-kikuc <[email protected]> * Modify consts to public for using in switch-case Signed-off-by: t-kikuc <[email protected]> * Fix a typo Signed-off-by: t-kikuc <[email protected]> * Refactor validation func Signed-off-by: t-kikuc <[email protected]> * Add a blank line in import Signed-off-by: t-kikuc <[email protected]> --------- Signed-off-by: t-kikuc <[email protected]> Signed-off-by: sZma5a <[email protected]>

* Support QuickSync for ECS Service Discovery pattern Signed-off-by: t-kikuc <[email protected]> * Update the test expectation of ELB pattern Signed-off-by: t-kikuc <[email protected]> * Add a test of QuickSync of ECS Service Discovery Signed-off-by: t-kikuc <[email protected]> * fix comment Signed-off-by: t-kikuc <[email protected]> * Add a description of Input values Signed-off-by: t-kikuc <[email protected]> * Add consts and validation of accessType Signed-off-by: t-kikuc <[email protected]> * Modify consts to public for using in switch-case Signed-off-by: t-kikuc <[email protected]> * Fix a typo Signed-off-by: t-kikuc <[email protected]> * Refactor validation func Signed-off-by: t-kikuc <[email protected]> * Add a blank line in import Signed-off-by: t-kikuc <[email protected]> --------- Signed-off-by: t-kikuc <[email protected]> Signed-off-by: 鈴木優耀 <[email protected]>

t-kikuc requested review from nghialv, khanhtc1202, kentakozuka and ffjlabo as code owners November 22, 2023 09:40

github-actions bot added the area/go label Nov 22, 2023

t-kikuc changed the title ~~Support QuickSync for ECS using Service Discovery~~ Support QuickSync for ECS without ELB or target groups Nov 22, 2023

t-kikuc mentioned this pull request Nov 22, 2023

[Draft] Add ECS Service Discovery funcs #4681

Closed

khanhtc1202 reviewed Nov 22, 2023

View reviewed changes

pkg/config/application_ecs.go Show resolved Hide resolved

khanhtc1202 reviewed Nov 22, 2023

View reviewed changes

khanhtc1202 reviewed Nov 27, 2023

View reviewed changes

pkg/config/application_ecs.go Outdated Show resolved Hide resolved

khanhtc1202 reviewed Nov 27, 2023

View reviewed changes

pkg/config/application_ecs.go Outdated Show resolved Hide resolved

khanhtc1202 reviewed Nov 27, 2023

View reviewed changes

pkg/config/application_ecs.go Show resolved Hide resolved

khanhtc1202 reviewed Nov 27, 2023

View reviewed changes

pkg/config/application_ecs.go Outdated Show resolved Hide resolved

t-kikuc force-pushed the support-ecs-service-discovery-sync branch from 903955c to ddc3d02 Compare November 27, 2023 05:26

t-kikuc added 9 commits November 27, 2023 15:18

Support QuickSync for ECS Service Discovery pattern

ab42d5d

Signed-off-by: t-kikuc <[email protected]>

Update the test expectation of ELB pattern

b1fd711

Signed-off-by: t-kikuc <[email protected]>

Add a test of QuickSync of ECS Service Discovery

476c3f6

Signed-off-by: t-kikuc <[email protected]>

fix comment

1a0b60d

Signed-off-by: t-kikuc <[email protected]>

Add a description of Input values

761b66b

Signed-off-by: t-kikuc <[email protected]>

Add consts and validation of accessType

88b0afe

Signed-off-by: t-kikuc <[email protected]>

Modify consts to public for using in switch-case

197507e

Signed-off-by: t-kikuc <[email protected]>

Fix a typo

5e2e676

Signed-off-by: t-kikuc <[email protected]>

Refactor validation func

840f2ce

Signed-off-by: t-kikuc <[email protected]>

t-kikuc force-pushed the support-ecs-service-discovery-sync branch from ddc3d02 to 840f2ce Compare November 27, 2023 06:21

khanhtc1202 reviewed Nov 27, 2023

View reviewed changes

pkg/app/piped/executor/ecs/deploy.go Show resolved Hide resolved

Add a blank line in import

8159ba7

Signed-off-by: t-kikuc <[email protected]>

khanhtc1202 approved these changes Nov 27, 2023

View reviewed changes

khanhtc1202 merged commit a64f250 into pipe-cd:master Nov 27, 2023
13 checks passed

t-kikuc mentioned this pull request Nov 30, 2023

Add an example of ECS canary for Service Discovery #4691

Merged

github-actions bot mentioned this pull request Dec 1, 2023

Release v0.45.4 #4697

Merged

github-actions bot mentioned this pull request Feb 6, 2024

Release v0.46.0 #4775

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support QuickSync for ECS without ELB or target groups #4685

Support QuickSync for ECS without ELB or target groups #4685

t-kikuc commented Nov 22, 2023

codecov bot commented Nov 22, 2023 •

edited

Loading

khanhtc1202 Nov 22, 2023

t-kikuc Nov 22, 2023

khanhtc1202 Nov 22, 2023

t-kikuc Nov 22, 2023

t-kikuc Nov 23, 2023

khanhtc1202 commented Nov 23, 2023

github-actions bot commented Nov 23, 2023

t-kikuc commented Nov 23, 2023

github-actions bot commented Nov 23, 2023

khanhtc1202 commented Nov 27, 2023

t-kikuc commented Nov 27, 2023

khanhtc1202 left a comment

Support QuickSync for ECS without ELB or target groups #4685

Support QuickSync for ECS without ELB or target groups #4685

Conversation

t-kikuc commented Nov 22, 2023

codecov bot commented Nov 22, 2023 • edited Loading

Codecov Report

khanhtc1202 Nov 22, 2023

Choose a reason for hiding this comment

t-kikuc Nov 22, 2023

Choose a reason for hiding this comment

khanhtc1202 Nov 22, 2023

Choose a reason for hiding this comment

t-kikuc Nov 22, 2023

Choose a reason for hiding this comment

t-kikuc Nov 23, 2023

Choose a reason for hiding this comment

khanhtc1202 commented Nov 23, 2023

github-actions bot commented Nov 23, 2023

PR Analysis

Main theme

PR summary

Type of PR

PR Feedback:

General suggestions

Code feedback

pkg/app/piped/executor/ecs/deploy.go

pkg/config/application_ecs.go

pkg/config/application_ecs_test.go

Security concerns:

t-kikuc commented Nov 23, 2023

github-actions bot commented Nov 23, 2023

PR Analysis

Main theme

PR summary

Type of PR

PR Feedback:

General suggestions

Code feedback

File: pkg/app/piped/executor/ecs/deploy.go

L9

L19-L25

File: pkg/config/application_ecs.go

L18-L27

L41-L42

File: pkg/config/application_ecs_test.go

L71-L77

Security concerns

khanhtc1202 commented Nov 27, 2023

t-kikuc commented Nov 27, 2023

khanhtc1202 left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 22, 2023 •

edited

Loading