feat(ECS): enable selection of listener rules #4668

moko-poi · 2023-11-11T15:25:41Z

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:

How are users affected by this change:

apiVersion: pipecd.dev/v1beta1
kind: ECSApp
input:
  taskDefinitionFile: taskdef.yaml
  serviceDefinitionFile: servicedef.yaml
  targetGroups:
    primary:
    canary:

Is this breaking change: no
How to migrate (if breaking change):

moko-poi · 2023-11-11T16:08:19Z

@khanhtc1202
I have recreated PR #4540 to incorporate the latest updates and resolve existing issues. Your review and feedback on the new PR would be greatly appreciated.

pkg/app/piped/platformprovider/ecs/client.go

codecov · 2023-11-13T07:55:11Z

Codecov Report

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

Comparison is base (8bad596) 30.82% compared to head (2650922) 30.75%.
Report is 2 commits behind head on master.

Files	Patch %	Lines
pkg/app/piped/platformprovider/ecs/client.go	0.00%	87 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4668      +/-   ##
==========================================
- Coverage   30.82%   30.75%   -0.08%     
==========================================
  Files         221      221              
  Lines       25935    26009      +74     
==========================================
+ Hits         7995     7999       +4     
- Misses      17289    17360      +71     
+ Partials      651      650       -1

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

khanhtc1202 · 2023-11-15T02:22:16Z

@moko-poi Please sign the commit with git commit -s to pass the CI

Signed-off-by: moko-poi <[email protected]>

* Format and make tests pass in tool/actions-plan-preview Signed-off-by: mi11km <[email protected]> * Use --piped-handle-timeout to make the timeout arg activate Signed-off-by: mi11km <[email protected]> * Add tests to model/notificationevent (pipe-cd#4631) * Add tests to model/notificationevent Signed-off-by: Kenta Kozuka <[email protected]> * Add Parallel() Signed-off-by: Kenta Kozuka <[email protected]> --------- Signed-off-by: Kenta Kozuka <[email protected]> Signed-off-by: mi11km <[email protected]> * add piped-handle-timeout(input args) Signed-off-by: mi11km <[email protected]> --------- Signed-off-by: mi11km <[email protected]> Signed-off-by: Kenta Kozuka <[email protected]> Co-authored-by: Kenta Kozuka <[email protected]> Co-authored-by: Khanh Tran <[email protected]> Signed-off-by: moko-poi <[email protected]>

Signed-off-by: moko-poi <[email protected]>

Signed-off-by: Yoshiki Fujikane <[email protected]> Signed-off-by: moko-poi <[email protected]>

moko-poi · 2023-11-15T03:08:00Z

@khanhtc1202
I've just signed the commit using git commit -s as you suggested. This should ensure that the commit passes the CI checks now.

Thank you for pointing this out

khanhtc1202 · 2023-11-17T05:52:07Z

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

+			return false
+		}
+
+		if err := client.ModifyListenerOrRule(ctx, currListenerArns, currListenerRuleArns, routingTrafficCfg); err != nil {


Instead of passing currListenerRuleArns to the function ModifyListenerOrRule (yes, the name is confusing), we should check currListenerRuleArns > 0 in this place, and call to ModifyRules if the condition check passed or call to ModifyListeners (the previous one) if the condition return false. wdyt? 🤔

@khanhtc1202
Thanks for the review. I've implemented the check for currListenerRuleArns and used ModifyRules or ModifyListeners accordingly to enhance rollback precision.
965ba41

Signed-off-by: moko-poi <[email protected]>

khanhtc1202 · 2023-11-24T03:01:50Z

pkg/app/piped/platformprovider/ecs/client.go

 	return nil
 }

+func (c *client) ModifyListenerOrRule(ctx context.Context, listenerRuleArns []string, routingTrafficCfg RoutingTrafficConfig) error {


Should rename this to ModifyRules

Thank you,Correspondence
3a4bea5

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

pkg/app/piped/platformprovider/ecs/ecs.go

Signed-off-by: moko-poi <[email protected]>

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

Signed-off-by: moko-poi <[email protected]>

moko-poi · 2023-11-24T09:38:28Z

@khanhtc1202
Fixed routing to target group when action type is forward.

khanhtc1202 · 2023-11-24T11:04:37Z

pkg/app/piped/platformprovider/ecs/client.go

+		// Check if the current action type is forward
+		for _, rule := range describelistenerOutput.Rules {
+			for _, action := range rule.Actions {
+				if action.Type == elbtypes.ActionTypeEnumForward {
+					// Call modifyListenerRule only if action type is forward
+					if err := modifyListener(ctx, listener); err != nil {
+						return err
+					}
+				}
+			}


This will delete all other rules of a given listener in case that listener contains rules of type ActionTypeEnumForward ? 🤔 So this code change means nothing, I guess. wdyt? @moko-poi

@khanhtc1202
2650922
Thank you for your review.
I have made the necessary adjustments to the ModifyListeners and ModifyRules functions to address the concerns. Now, these functions only modify actions of type ActionTypeEnumForward and preserve all other action types, ensuring that existing listener and rule configurations are not inadvertently altered.

…fication Signed-off-by: moko-poi <[email protected]>

khanhtc1202

Thanks @moko-poi 🙌
I will merge this and keep working based on this PR 👍

* feat(ECS): enable selection of listener rules Signed-off-by: moko-poi <[email protected]> * Update pkg/app/piped/platformprovider/ecs/client.go Signed-off-by: moko-poi <[email protected]> * Make actions plan preview handle timeout option (#4648) * Format and make tests pass in tool/actions-plan-preview Signed-off-by: mi11km <[email protected]> * Use --piped-handle-timeout to make the timeout arg activate Signed-off-by: mi11km <[email protected]> * Add tests to model/notificationevent (#4631) * Add tests to model/notificationevent Signed-off-by: Kenta Kozuka <[email protected]> * Add Parallel() Signed-off-by: Kenta Kozuka <[email protected]> --------- Signed-off-by: Kenta Kozuka <[email protected]> Signed-off-by: mi11km <[email protected]> * add piped-handle-timeout(input args) Signed-off-by: mi11km <[email protected]> --------- Signed-off-by: mi11km <[email protected]> Signed-off-by: Kenta Kozuka <[email protected]> Co-authored-by: Kenta Kozuka <[email protected]> Co-authored-by: Khanh Tran <[email protected]> Signed-off-by: moko-poi <[email protected]> * Increase limit of stale action operations (#4671) Signed-off-by: moko-poi <[email protected]> * Add docs for k8s annotations (#4673) Signed-off-by: Yoshiki Fujikane <[email protected]> Signed-off-by: moko-poi <[email protected]> * feat: add modifylisner and modifyrule Signed-off-by: moko-poi <[email protected]> * fix: ModifyRules Signed-off-by: moko-poi <[email protected]> * fix: rollback Signed-off-by: moko-poi <[email protected]> * fix: routing Signed-off-by: moko-poi <[email protected]> * fix: action.Type == elbtypes.ActionTypeEnumForward Signed-off-by: moko-poi <[email protected]> * fix: Update ModifyListeners and ModifyRules for Selective Action Modification Signed-off-by: moko-poi <[email protected]> --------- Signed-off-by: moko-poi <[email protected]> Signed-off-by: mi11km <[email protected]> Signed-off-by: Kenta Kozuka <[email protected]> Signed-off-by: Yoshiki Fujikane <[email protected]> Co-authored-by: Khanh Tran <[email protected]> Co-authored-by: Masafumi Ikeyama <[email protected]> Co-authored-by: Kenta Kozuka <[email protected]> Co-authored-by: Yoshiki Fujikane <[email protected]>

* feat(ECS): enable selection of listener rules (#4668) * feat(ECS): enable selection of listener rules Signed-off-by: moko-poi <[email protected]> * Update pkg/app/piped/platformprovider/ecs/client.go Signed-off-by: moko-poi <[email protected]> * Make actions plan preview handle timeout option (#4648) * Format and make tests pass in tool/actions-plan-preview Signed-off-by: mi11km <[email protected]> * Use --piped-handle-timeout to make the timeout arg activate Signed-off-by: mi11km <[email protected]> * Add tests to model/notificationevent (#4631) * Add tests to model/notificationevent Signed-off-by: Kenta Kozuka <[email protected]> * Add Parallel() Signed-off-by: Kenta Kozuka <[email protected]> --------- Signed-off-by: Kenta Kozuka <[email protected]> Signed-off-by: mi11km <[email protected]> * add piped-handle-timeout(input args) Signed-off-by: mi11km <[email protected]> --------- Signed-off-by: mi11km <[email protected]> Signed-off-by: Kenta Kozuka <[email protected]> Co-authored-by: Kenta Kozuka <[email protected]> Co-authored-by: Khanh Tran <[email protected]> Signed-off-by: moko-poi <[email protected]> * Increase limit of stale action operations (#4671) Signed-off-by: moko-poi <[email protected]> * Add docs for k8s annotations (#4673) Signed-off-by: Yoshiki Fujikane <[email protected]> Signed-off-by: moko-poi <[email protected]> * feat: add modifylisner and modifyrule Signed-off-by: moko-poi <[email protected]> * fix: ModifyRules Signed-off-by: moko-poi <[email protected]> * fix: rollback Signed-off-by: moko-poi <[email protected]> * fix: routing Signed-off-by: moko-poi <[email protected]> * fix: action.Type == elbtypes.ActionTypeEnumForward Signed-off-by: moko-poi <[email protected]> * fix: Update ModifyListeners and ModifyRules for Selective Action Modification Signed-off-by: moko-poi <[email protected]> --------- Signed-off-by: moko-poi <[email protected]> Signed-off-by: mi11km <[email protected]> Signed-off-by: Kenta Kozuka <[email protected]> Signed-off-by: Yoshiki Fujikane <[email protected]> Co-authored-by: Khanh Tran <[email protected]> Co-authored-by: Masafumi Ikeyama <[email protected]> Co-authored-by: Kenta Kozuka <[email protected]> Co-authored-by: Yoshiki Fujikane <[email protected]> * Remove ModifyRules interface (#4688) --------- Signed-off-by: moko-poi <[email protected]> Signed-off-by: mi11km <[email protected]> Signed-off-by: Kenta Kozuka <[email protected]> Signed-off-by: Yoshiki Fujikane <[email protected]> Co-authored-by: Shun Takahashi <[email protected]> Co-authored-by: Masafumi Ikeyama <[email protected]> Co-authored-by: Kenta Kozuka <[email protected]> Co-authored-by: Yoshiki Fujikane <[email protected]>

* feat(ECS): enable selection of listener rules Signed-off-by: moko-poi <[email protected]> * Update pkg/app/piped/platformprovider/ecs/client.go Signed-off-by: moko-poi <[email protected]> * Make actions plan preview handle timeout option (pipe-cd#4648) * Format and make tests pass in tool/actions-plan-preview Signed-off-by: mi11km <[email protected]> * Use --piped-handle-timeout to make the timeout arg activate Signed-off-by: mi11km <[email protected]> * Add tests to model/notificationevent (pipe-cd#4631) * Add tests to model/notificationevent Signed-off-by: Kenta Kozuka <[email protected]> * Add Parallel() Signed-off-by: Kenta Kozuka <[email protected]> --------- Signed-off-by: Kenta Kozuka <[email protected]> Signed-off-by: mi11km <[email protected]> * add piped-handle-timeout(input args) Signed-off-by: mi11km <[email protected]> --------- Signed-off-by: mi11km <[email protected]> Signed-off-by: Kenta Kozuka <[email protected]> Co-authored-by: Kenta Kozuka <[email protected]> Co-authored-by: Khanh Tran <[email protected]> Signed-off-by: moko-poi <[email protected]> * Increase limit of stale action operations (pipe-cd#4671) Signed-off-by: moko-poi <[email protected]> * Add docs for k8s annotations (pipe-cd#4673) Signed-off-by: Yoshiki Fujikane <[email protected]> Signed-off-by: moko-poi <[email protected]> * feat: add modifylisner and modifyrule Signed-off-by: moko-poi <[email protected]> * fix: ModifyRules Signed-off-by: moko-poi <[email protected]> * fix: rollback Signed-off-by: moko-poi <[email protected]> * fix: routing Signed-off-by: moko-poi <[email protected]> * fix: action.Type == elbtypes.ActionTypeEnumForward Signed-off-by: moko-poi <[email protected]> * fix: Update ModifyListeners and ModifyRules for Selective Action Modification Signed-off-by: moko-poi <[email protected]> --------- Signed-off-by: moko-poi <[email protected]> Signed-off-by: mi11km <[email protected]> Signed-off-by: Kenta Kozuka <[email protected]> Signed-off-by: Yoshiki Fujikane <[email protected]> Co-authored-by: Khanh Tran <[email protected]> Co-authored-by: Masafumi Ikeyama <[email protected]> Co-authored-by: Kenta Kozuka <[email protected]> Co-authored-by: Yoshiki Fujikane <[email protected]>

* feat(ECS): enable selection of listener rules Signed-off-by: moko-poi <[email protected]> * Update pkg/app/piped/platformprovider/ecs/client.go Signed-off-by: moko-poi <[email protected]> * Make actions plan preview handle timeout option (pipe-cd#4648) * Format and make tests pass in tool/actions-plan-preview Signed-off-by: mi11km <[email protected]> * Use --piped-handle-timeout to make the timeout arg activate Signed-off-by: mi11km <[email protected]> * Add tests to model/notificationevent (pipe-cd#4631) * Add tests to model/notificationevent Signed-off-by: Kenta Kozuka <[email protected]> * Add Parallel() Signed-off-by: Kenta Kozuka <[email protected]> --------- Signed-off-by: Kenta Kozuka <[email protected]> Signed-off-by: mi11km <[email protected]> * add piped-handle-timeout(input args) Signed-off-by: mi11km <[email protected]> --------- Signed-off-by: mi11km <[email protected]> Signed-off-by: Kenta Kozuka <[email protected]> Co-authored-by: Kenta Kozuka <[email protected]> Co-authored-by: Khanh Tran <[email protected]> Signed-off-by: moko-poi <[email protected]> * Increase limit of stale action operations (pipe-cd#4671) Signed-off-by: moko-poi <[email protected]> * Add docs for k8s annotations (pipe-cd#4673) Signed-off-by: Yoshiki Fujikane <[email protected]> Signed-off-by: moko-poi <[email protected]> * feat: add modifylisner and modifyrule Signed-off-by: moko-poi <[email protected]> * fix: ModifyRules Signed-off-by: moko-poi <[email protected]> * fix: rollback Signed-off-by: moko-poi <[email protected]> * fix: routing Signed-off-by: moko-poi <[email protected]> * fix: action.Type == elbtypes.ActionTypeEnumForward Signed-off-by: moko-poi <[email protected]> * fix: Update ModifyListeners and ModifyRules for Selective Action Modification Signed-off-by: moko-poi <[email protected]> --------- Signed-off-by: moko-poi <[email protected]> Signed-off-by: mi11km <[email protected]> Signed-off-by: Kenta Kozuka <[email protected]> Signed-off-by: Yoshiki Fujikane <[email protected]> Co-authored-by: Khanh Tran <[email protected]> Co-authored-by: Masafumi Ikeyama <[email protected]> Co-authored-by: Kenta Kozuka <[email protected]> Co-authored-by: Yoshiki Fujikane <[email protected]> Signed-off-by: sZma5a <[email protected]>

* feat(ECS): enable selection of listener rules Signed-off-by: moko-poi <[email protected]> * Update pkg/app/piped/platformprovider/ecs/client.go Signed-off-by: moko-poi <[email protected]> * Make actions plan preview handle timeout option (pipe-cd#4648) * Format and make tests pass in tool/actions-plan-preview Signed-off-by: mi11km <[email protected]> * Use --piped-handle-timeout to make the timeout arg activate Signed-off-by: mi11km <[email protected]> * Add tests to model/notificationevent (pipe-cd#4631) * Add tests to model/notificationevent Signed-off-by: Kenta Kozuka <[email protected]> * Add Parallel() Signed-off-by: Kenta Kozuka <[email protected]> --------- Signed-off-by: Kenta Kozuka <[email protected]> Signed-off-by: mi11km <[email protected]> * add piped-handle-timeout(input args) Signed-off-by: mi11km <[email protected]> --------- Signed-off-by: mi11km <[email protected]> Signed-off-by: Kenta Kozuka <[email protected]> Co-authored-by: Kenta Kozuka <[email protected]> Co-authored-by: Khanh Tran <[email protected]> Signed-off-by: moko-poi <[email protected]> * Increase limit of stale action operations (pipe-cd#4671) Signed-off-by: moko-poi <[email protected]> * Add docs for k8s annotations (pipe-cd#4673) Signed-off-by: Yoshiki Fujikane <[email protected]> Signed-off-by: moko-poi <[email protected]> * feat: add modifylisner and modifyrule Signed-off-by: moko-poi <[email protected]> * fix: ModifyRules Signed-off-by: moko-poi <[email protected]> * fix: rollback Signed-off-by: moko-poi <[email protected]> * fix: routing Signed-off-by: moko-poi <[email protected]> * fix: action.Type == elbtypes.ActionTypeEnumForward Signed-off-by: moko-poi <[email protected]> * fix: Update ModifyListeners and ModifyRules for Selective Action Modification Signed-off-by: moko-poi <[email protected]> --------- Signed-off-by: moko-poi <[email protected]> Signed-off-by: mi11km <[email protected]> Signed-off-by: Kenta Kozuka <[email protected]> Signed-off-by: Yoshiki Fujikane <[email protected]> Co-authored-by: Khanh Tran <[email protected]> Co-authored-by: Masafumi Ikeyama <[email protected]> Co-authored-by: Kenta Kozuka <[email protected]> Co-authored-by: Yoshiki Fujikane <[email protected]> Signed-off-by: 鈴木優耀 <[email protected]>

moko-poi requested review from ffjlabo, kentakozuka, khanhtc1202 and nghialv as code owners November 11, 2023 15:25

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

moko-poi mentioned this pull request Nov 11, 2023

feat(ECS): enable selection of listener rules #4540

Closed

khanhtc1202 reviewed Nov 13, 2023

View reviewed changes

pkg/app/piped/platformprovider/ecs/client.go Outdated Show resolved Hide resolved

moko-poi requested a review from khanhtc1202 November 13, 2023 14:01

moko-poi and others added 5 commits November 15, 2023 12:05

feat(ECS): enable selection of listener rules

9e78352

Signed-off-by: moko-poi <[email protected]>

Update pkg/app/piped/platformprovider/ecs/client.go

41b67d6

Signed-off-by: moko-poi <[email protected]>

Increase limit of stale action operations (pipe-cd#4671)

5ea306a

Signed-off-by: moko-poi <[email protected]>

Add docs for k8s annotations (pipe-cd#4673)

5b3ac3f

Signed-off-by: Yoshiki Fujikane <[email protected]> Signed-off-by: moko-poi <[email protected]>

moko-poi force-pushed the feat/enable-selection-of-listener-rule branch from 1c26321 to 5b3ac3f Compare November 15, 2023 03:06

github-actions bot added area/build area/tool labels Nov 15, 2023

Merge branch 'master' into feat/enable-selection-of-listener-rule

e7cb581

github-actions bot removed area/build area/tool labels Nov 16, 2023

khanhtc1202 reviewed Nov 17, 2023

View reviewed changes

khanhtc1202 and others added 2 commits November 22, 2023 13:45

Merge branch 'master' into feat/enable-selection-of-listener-rule

a25602c

feat: add modifylisner and modifyrule

965ba41

Signed-off-by: moko-poi <[email protected]>

moko-poi requested a review from khanhtc1202 November 24, 2023 01:50

khanhtc1202 reviewed Nov 24, 2023

View reviewed changes

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

khanhtc1202 reviewed Nov 24, 2023

View reviewed changes

pkg/app/piped/platformprovider/ecs/ecs.go Show resolved Hide resolved

moko-poi added 2 commits November 24, 2023 13:13

fix: ModifyRules

3a4bea5

Signed-off-by: moko-poi <[email protected]>

fix: rollback

0b01246

Signed-off-by: moko-poi <[email protected]>

moko-poi requested a review from khanhtc1202 November 24, 2023 04:20

khanhtc1202 reviewed Nov 24, 2023

View reviewed changes

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

moko-poi added 2 commits November 24, 2023 17:40

fix: routing

abd12ca

Signed-off-by: moko-poi <[email protected]>

fix: action.Type == elbtypes.ActionTypeEnumForward

38855ce

Signed-off-by: moko-poi <[email protected]>

khanhtc1202 reviewed Nov 24, 2023

View reviewed changes

fix: Update ModifyListeners and ModifyRules for Selective Action Modi…

2650922

…fication Signed-off-by: moko-poi <[email protected]>

khanhtc1202 approved these changes Nov 28, 2023

View reviewed changes

khanhtc1202 merged commit 426cb04 into pipe-cd:master Nov 28, 2023

This was referenced Nov 28, 2023

Remove ModifyRules interface #4688

Merged

Cherry-pick #4668 #4688 #4689

Merged

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

Release v0.45.4 #4697

Merged

t-kikuc mentioned this pull request Dec 17, 2023

[ECS] Enable selection of listener rules to modify #4726

Closed

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

Release v0.46.0 #4775

Merged

feat(ECS): enable selection of listener rules #4668

feat(ECS): enable selection of listener rules #4668

Uh oh!

Conversation

moko-poi commented Nov 11, 2023

Uh oh!

moko-poi commented Nov 11, 2023

Uh oh!

Uh oh!

codecov bot commented Nov 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

khanhtc1202 commented Nov 15, 2023

Uh oh!

moko-poi commented Nov 15, 2023

Uh oh!

khanhtc1202 Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

moko-poi Nov 24, 2023

Choose a reason for hiding this comment

Uh oh!

khanhtc1202 Nov 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moko-poi Nov 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

moko-poi commented Nov 24, 2023

Uh oh!

khanhtc1202 Nov 24, 2023

Choose a reason for hiding this comment

Uh oh!

moko-poi Nov 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khanhtc1202 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Nov 13, 2023 •

edited

Loading

khanhtc1202 Nov 24, 2023 •

edited

Loading

moko-poi Nov 26, 2023 •

edited

Loading