[ECS] Support Plan Preview for ECS #4881

t-kikuc · 2024-04-19T00:32:11Z

What this PR does / why we need it:

Support Plan Preview for ECS
Including Diff() func, which compares manifests(i.e. serviceDef&taskDef in ECS) for non-k8s resources like Lambda

Which issue(s) this PR fixes:

Fixes #4870

Does this PR introduce a user-facing change?: no

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

How it works:

Plan Preview is a feature for visualizing model-level diff, not file-level diff(Files Changed on GitHub covers it).
So we can ignore following points in the output:

Each key becomes UpperCamelCase.
e.g. image in taskdef.yaml ->Image in planpreview result
The result also shows attributes that a user did not specify in taskdef.yaml or servicedef.yaml.
e.g. EnableExecuteCommand is shown above but I did not specify it.

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

codecov · 2024-04-19T00:36:36Z

Codecov Report

Attention: Patch coverage is 32.78689% with 164 lines in your changes are missing coverage. Please review.

Project coverage is 29.30%. Comparing base (fa0708b) to head (a161cba).
Report is 3 commits behind head on master.

Files	Patch %	Lines
pkg/app/piped/planpreview/ecsdiff.go	0.00%	87 Missing ⚠️
pkg/app/piped/platformprovider/ecs/cache.go	0.00%	30 Missing ⚠️
pkg/diff/renderer.go	56.25%	14 Missing and 7 partials ⚠️
pkg/app/piped/platformprovider/ecs/diff.go	75.51%	7 Missing and 5 partials ⚠️
pkg/diff/diff.go	57.14%	8 Missing and 4 partials ⚠️
pkg/app/piped/planpreview/builder.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4881      +/-   ##
==========================================
+ Coverage   29.23%   29.30%   +0.07%     
==========================================
  Files         318      321       +3     
  Lines       40591    40835     +244     
==========================================
+ Hits        11868    11968     +100     
- Misses      27784    27910     +126     
- Partials      939      957      +18

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

khanhtc1202 · 2024-04-22T10:17:59Z

pkg/diff/diffbycommand.go

+// DiffBytesByCommand runs the command to compare the given bytes.
+func DiffBytesByCommand(command string, oldBytes, newBytes []byte) ([]byte, error) {


What is the point of separate this function from DiffByCommand? 🤔

I fixed to merge DiffBytesByCommand() into DiffByCommand().

I used DiffBytesByCommand() from outside in previous commits, but it is not used now.

khanhtc1202 · 2024-04-22T10:33:00Z

pkg/diff/diff.go

@@ -97,6 +98,42 @@ func DiffUnstructureds(x, y unstructured.Unstructured, key string, opts ...Optio
 	return d.result, nil
 }

+// DiffStructs calulates the diff between two struct objects,
+// which are not k8s manifests.


I can imagine this related to this line in your PR description.

Including Diff() func, which compares manifests(i.e. serviceDef&taskDef in ECS) for non-k8s resources like Lambda

But it is still hard to imagine when to use DiffUnstructureds and DiffStructs. Maybe we need a better name, or we need to add comments that describe these two functions better.

Maybe we also should rename this to DiffStructureds either ways 🤔

Thanks, I also felt the name DiffStructs was unclear.

First, I renamed it to DiffStructureds.

Second, I added comments about which func to use in both DiffUnstructureds and DiffStructureds.
commit: 77dcef7

khanhtc1202 · 2024-04-22T10:44:02Z

pkg/diff/diff.go

+		opt(d)
+	}
+
+	d.initIgnoredPaths(key)


Not sure this would work? 🤔 Or could you introduce the possible key for this? Since this key in case of k8s manifests could look like this https://pipecd.dev/docs-v0.47.x/user-guide/managing-application/configuration-drift-detection/#ignore-drift-detection-for-specific-fields

I'm sorry, you're right.
It did not work, so I deleted them in dd40abb

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

t-kikuc · 2024-04-23T04:38:13Z

@khanhtc1202 Thank you for your reviewing, I fixed 3 points.

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

t-kikuc · 2024-04-25T09:41:51Z

@khanhtc1202
I fixed:

Moved LoadECSManifest() to the test code
Modified tests of DiffByCommand()
Renamed 'ECSManifest' to 'ECSManifests'

ffjlabo

Thank you for the great work! I left some nit and imo comment!

ffjlabo · 2024-04-26T01:46:53Z

pkg/app/piped/planpreview/ecsdiff.go

+	summary := fmt.Sprintf("%d changes were detected", len(result.Diff.Nodes()))
+	details := result.Render(provider.DiffRenderOptions{
+		UseDiffCommand: true,
+	})
+	fmt.Fprintf(buf, "--- Last Deploy\n+++ Head Commit\n\n%s\n", details)
+
+	return &diffResult{
+		summary: summary,
+	}, nil


[nit] maybe be able to reduce the variable

Suggested change

summary := fmt.Sprintf("%d changes were detected", len(result.Diff.Nodes()))

details := result.Render(provider.DiffRenderOptions{

UseDiffCommand: true,

})

fmt.Fprintf(buf, "--- Last Deploy\n+++ Head Commit\n\n%s\n", details)

return &diffResult{

summary: summary,

}, nil

details := result.Render(provider.DiffRenderOptions{

UseDiffCommand: true,

})

fmt.Fprintf(buf, "--- Last Deploy\n+++ Head Commit\n\n%s\n", details)

return &diffResult{

summary: fmt.Sprintf("%d changes were detected", len(result.Diff.Nodes())),

}, nil

thanks, I fixed

3fee826

ffjlabo · 2024-04-26T02:55:11Z

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

+func diffByCommand(command string, old, new ECSManifests) ([]byte, error) {
+	taskDiff, err := diff.DiffByCommand(command, old.TaskDefinition, new.TaskDefinition)
+	if err != nil {
+		return nil, err
+	}
+
+	serviceDiff, err := diff.DiffByCommand(command, old.ServiceDefinition, new.ServiceDefinition)
+	if err != nil {
+		return nil, err
+	}
+
+	return bytes.Join([][]byte{
+		[]byte("# 1. ServiceDefinition"),
+		serviceDiff,
+		[]byte("\n# 2. TaskDefinition"),
+		taskDiff,
+	}, []byte("\n")), nil
+}


[IMO]
It would be better to define it as renderByCommand.
I feel the responsibility of the method is to render the diffResult.

thanks, I fixed it. 0cb5d29

renderByCommand is better especially because this func contains the following code.

return bytes.Join([][]byte{ []byte("# 1. ServiceDefinition"), serviceDiff, []byte("\n# 2. TaskDefinition"), taskDiff, }, []byte("\n")), nil

ffjlabo · 2024-04-26T03:07:49Z

pkg/diff/diffbycommand.go

+// DiffByCommand converts the given objects into yaml and then runs the command to compare them.
+func DiffByCommand(command string, old, new interface{}) ([]byte, error) {


[IMO]
It would be better to define it as renderByCommand.
I feel the responsibility of the method is to render the diffResult.

Similar to the reason above, how about renaming it as RenderByCommand and moving it in the render.go like below↓?
https://github.com/pipe-cd/pipecd/blob/master/pkg/diff/renderer.go#L54

I think the DiffXX methods in the diff pkg return diff.Result and have the responsibility to extract the differences. It is ideal to unify the interface and the responsivility as much as possible.

WDYT? I want your opinion @t-kikuc @khanhtc1202

@ffjlabo
Thanks, I agree with you.
TBH, when I referred to CloudRun's diff.go, I thought it was confusing and should be renamed and moved.
cf. https://github.com/pipe-cd/pipecd/blob/master/pkg/app/piped/platformprovider/cloudrun/diff.go

ffjlabo · 2024-04-26T03:19:28Z

pkg/diff/diffbycommand.go

+func DiffByCommand(command string, old, new interface{}) ([]byte, error) {
+	oldBytes, err := yaml.Marshal(old)


[nit] Check whether the command exists or not.

thanks, I fixed it. c9764e4

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

…into ecs-plan-preview Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

ffjlabo

@t-kikuc thanks! added one nit comment for test! I'll approve after the fix :)

ffjlabo · 2024-04-30T01:03:45Z

pkg/diff/renderer_test.go

+			if tc.expectErr {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}


[nit]

Suggested change

if tc.expectErr {

assert.Error(t, err)

} else {

assert.NoError(t, err)

}

assert.Equal(t, tc.expectErr, err != nil)

@ffjlabo
Thanks!!! I fixed it: 64efe0a

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

…into ecs-plan-preview Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

ffjlabo

Thank you 🚀

khanhtc1202

Nice work, can't wait to see this on prod 🚀

t-kikuc added 19 commits April 8, 2024 20:37

Impl ECS's Diff func of taskDef and serviceDef

42aa504

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Add ECSManifest's cache

52d89e9

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Impl ECSDiff for planpreview

841459b

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Enable ECS PlanPreview

8c2157d

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Draft: diff test

f99bebe

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Add and fix tests of diff with TODO of lowerCamelCase

8190daa

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

fix tests: add params to servicedef

98b740b

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Remove wip comments

43d3ff8

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Skip summarizing if no changes were detected

b416577

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Add tags to test data

5f4f2b9

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

removed an unnecessary test case

46cff87

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Clean comments, removing TODO

5325c36

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

remove an unnecessary blank line in the test data

0b33d5a

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Modify ApiVersion and Kind to apparent dummy

00c9dd8

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Add DiffStructs() for comparing non-k8s manifests

c9f2975

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

removed unstructured.Unstructured from ECS

7613dd5

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Separate DiffByCommand() to diff package

17b8dfe

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Fix diff render output: split ServiceDef and TaskDef sections

6b07291

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Add a testcase to planpreview_test for ECS

9ff2fcf

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

t-kikuc added the kind/enhancement New feature or request label Apr 19, 2024

t-kikuc requested review from nghialv, khanhtc1202, kentakozuka and ffjlabo as code owners April 19, 2024 00:32

github-actions bot added the area/go label Apr 19, 2024

Merge branch 'master' into ecs-plan-preview

323b4bb

khanhtc1202 reviewed Apr 22, 2024

View reviewed changes

t-kikuc added 2 commits April 23, 2024 10:53

rename func to DiffStructureds()

59f3171

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

add comment of which func to use

77dcef7

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

t-kikuc added 5 commits April 24, 2024 17:33

Removed LoadECSManifest() func

9279185

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Rename test func

db54c29

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Add tests of diffbycommand

eca1b74

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Removed duplicated tests

46233d0

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Rename ECSManifest to ECSManifests

97405b4

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Merge branch 'master' into ecs-plan-preview

33fdec1

ffjlabo reviewed Apr 26, 2024

View reviewed changes

t-kikuc and others added 7 commits April 26, 2024 13:11

Removed an unnecessary variable

3fee826

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Renamed func to 'renderByCommand'

0cb5d29

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Add check of existence of the command

c9764e4

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Rename func to 'RenderByCommand'

55b89b6

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Moved func 'RenderByCommand()' to renderer.go

c38c07b

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Merge branch 'ecs-plan-preview' of https://github.com/pipe-cd/pipecd …

6f542c6

…into ecs-plan-preview Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Merge branch 'master' into ecs-plan-preview

e252b8d

ffjlabo reviewed Apr 30, 2024

View reviewed changes

t-kikuc added 2 commits April 30, 2024 10:10

Fix nits: removed unnecessary if-state

64efe0a

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Merge branch 'ecs-plan-preview' of https://github.com/pipe-cd/pipecd …

a161cba

…into ecs-plan-preview Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

ffjlabo approved these changes Apr 30, 2024

View reviewed changes

khanhtc1202 approved these changes Apr 30, 2024

View reviewed changes

t-kikuc merged commit b17c25b into master Apr 30, 2024
14 checks passed

t-kikuc deleted the ecs-plan-preview branch April 30, 2024 11:08

t-kikuc mentioned this pull request May 9, 2024

[doc] Update the feature status of ECS Plan Preview to Alpha #4907

Merged

github-actions bot mentioned this pull request May 13, 2024

Release v0.47.2 #4911

Merged

github-actions bot mentioned this pull request Jun 12, 2024

Release v0.47.3 #4967

Closed

github-actions bot mentioned this pull request Jul 4, 2024

Release v0.48.0 #5023

Merged

t-kikuc mentioned this pull request Jul 16, 2024

Support Plan Preview for Lambda #5046

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ECS] Support Plan Preview for ECS #4881

[ECS] Support Plan Preview for ECS #4881

t-kikuc commented Apr 19, 2024

codecov bot commented Apr 19, 2024 •

edited

Loading

khanhtc1202 Apr 22, 2024

t-kikuc Apr 23, 2024 •

edited

Loading

khanhtc1202 Apr 22, 2024

khanhtc1202 Apr 22, 2024

t-kikuc Apr 23, 2024 •

edited

Loading

khanhtc1202 Apr 22, 2024

t-kikuc Apr 23, 2024

t-kikuc commented Apr 23, 2024

t-kikuc commented Apr 25, 2024

ffjlabo left a comment

ffjlabo Apr 26, 2024

t-kikuc Apr 26, 2024

ffjlabo Apr 26, 2024

t-kikuc Apr 26, 2024

ffjlabo Apr 26, 2024

t-kikuc Apr 26, 2024

ffjlabo Apr 26, 2024

t-kikuc Apr 26, 2024

ffjlabo left a comment

ffjlabo Apr 30, 2024

t-kikuc Apr 30, 2024

ffjlabo left a comment

khanhtc1202 left a comment

		// DiffBytesByCommand runs the command to compare the given bytes.
		func DiffBytesByCommand(command string, oldBytes, newBytes []byte) ([]byte, error) {

		// DiffByCommand converts the given objects into yaml and then runs the command to compare them.
		func DiffByCommand(command string, old, new interface{}) ([]byte, error) {

		func DiffByCommand(command string, old, new interface{}) ([]byte, error) {
		oldBytes, err := yaml.Marshal(old)

[ECS] Support Plan Preview for ECS #4881

[ECS] Support Plan Preview for ECS #4881

Conversation

t-kikuc commented Apr 19, 2024

codecov bot commented Apr 19, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

t-kikuc Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-kikuc Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-kikuc commented Apr 23, 2024

t-kikuc commented Apr 25, 2024

ffjlabo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffjlabo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffjlabo left a comment

Choose a reason for hiding this comment

khanhtc1202 left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 19, 2024 •

edited

Loading

t-kikuc Apr 23, 2024 •

edited

Loading

t-kikuc Apr 23, 2024 •

edited

Loading