Keep new line at end of file in yamlprocessor #4033

kurochan · 2022-11-30T17:51:55Z

What this PR does / why we need it:
Currently, yamlprocessor doesn't respect new line at end of file if the input file contains it or not.
This behavior causes unnecessary differences and should be corrected.

For example, EventWatcher generates these changes.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

Keep new line at end of file in yamlprocessor

khanhtc1202 · 2022-12-01T00:16:08Z

pkg/yamlprocessor/yamlprocessor_test.go

@@ -253,6 +253,16 @@ foo:
  - baz`),
 			wantErr: false,
 		},
+		{
+			name:    "new line at end of yml given",
+			yml:     `foo: bar


what if raw yaml has more than one \n at the end of file 🤔

@khanhtc1202 It's just as you said...

How about simply always adding a new line at the EOF?

According to the POSIX Stndard, The definition of a LINE is

A sequence of zero or more non- characters plus a terminating character.

https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline

In that case, I guess diff will be shown if the input file does not contain a new line at EOF, thus the idea of making no diff caused by a new line at EOF is not guaranteed.

I think we should update the logic to count new lines at EOF and add (if necessary) new line(s) to the end of parsed file, wdyt? 👀

My suggestion is to always add single newline to the end of the output file, whether or not there are some newlines at the end of the source file.

I understand your suggestion 👍 How about changing it to: add a new line to the EOF in case of there is no \n at EOF? If we just add a new line anyway (regardless there is a new line at the EOF or not), in case there is already one \n at EOF, we added an unnecessary new line and could cause a diff shown, wdyt?

I think there is no \n at EOF in generated yaml. Because yamlprocessor calls here (https://github.com/goccy/go-yaml/blob/master/ast/ast.go#L610 ).

return strings.Join(doc, "\n")

Based on this, which of the following should I do?

add a new line to the EOF

add a new line to the EOF in case of there is no \n at EOF

Make Pull Request to goccy/go-yaml to append \n at EOF

Do 2 and 3

I think (2) is good enough for pipecd, about (3) may need discuss on goccy/go-yaml repo

OK, I'll fix it in the way of (2).

And (3) will discuss on goccy/go-yaml#329 .

changed cd484f0

pkg/yamlprocessor/yamlprocessor.go

khanhtc1202 · 2022-12-05T14:29:43Z

@kurochan there is a failed test, pls re-check it 😄

kurochan · 2022-12-08T03:25:03Z

@khanhtc1202
I'm struggling on test fail on pkg/app/piped/executor/kubernetes/kubernetes_test.go .
The following differences appear and I haven't come up with a good fix yet 🤔

Diff:                                
--- Expected                                
+++ Actual                                
@@ -2,3 +2,3 @@                                
 data:                                
-  envoy-config: |-                                
+  envoy-config: |                                
     admin:                                
TestPatchManifest/multi_ops_with_a_given_field

Do you have any ideas to fix this differences?

khanhtc1202 · 2022-12-08T09:02:25Z

@kurochan Probably it's caused by this convention
https://stackoverflow.com/questions/3790454/how-do-i-break-a-string-in-yaml-over-multiple-lines#:~:text=Block%20styles%20with%20block%20chomping%20indicator
So I think your change should support all YAML convention includes: >, | and its mix with -, + etc

kurochan · 2022-12-08T18:09:33Z

pkg/app/piped/platformprovider/kubernetes/manifest.go

@@ -224,12 +224,17 @@ func ParseManifests(data string) ([]Manifest, error) {
 		manifests = make([]Manifest, 0, len(parts))
 	)

-	for _, part := range parts {
+	newLineAtEOF := strings.HasSuffix(data, "\n")
+	for i, part := range parts {
 		// Ignore all the cases where no content between separator.
 		part = strings.TrimSpace(part)


strings.TrimSpace(str) removes trailing \n, so \n at EOF is removed unexpectedly.

Looks like in case of the yaml file contains more than one empty line at the end of file, only one empty line is remained thus the diff will be shown, isn't it? In that case, lets add test to demonstrate that and I will merge this PR 🙌

Thanks for pointing that out. I have fixed it and added a test case. 0e250b1

khanhtc1202

Nice work! Thanks for the contribution 🙌

* Keep new line at end of file in yamlprocessor * add a new line to the EOF in case of there is no at EOF * update comment * update eventwatcher_test * consider new line at EOF on parsing kubernetes manifest * update go-yaml v1.9.8 * remove TODO because PR of go-yaml was merged * consider to handle multiple new line at EOF * remove indent vioration of tab

… (#4128) * Keep new line at end of file in yamlprocessor (#4033) * Keep new line at end of file in yamlprocessor * add a new line to the EOF in case of there is no at EOF * update comment * update eventwatcher_test * consider new line at EOF on parsing kubernetes manifest * update go-yaml v1.9.8 * remove TODO because PR of go-yaml was merged * consider to handle multiple new line at EOF * remove indent vioration of tab * support standalone ECS task (#4084) Fixes #2734 * Small lint fix (#4097) * fix null pointer (#4102) * fix null pointer * apply review * Add example for ecs standalone task (#4104) * add example for ecs standalone task * add example for ecs standalone task * wip * add yaml example to docs. * add example for ecs standalone task * wip * add yaml example to docs. * apply review * Update docs/content/en/docs-dev/user-guide/managing-application/defining-app-configuration/ecs.md Co-authored-by: Khanh Tran <32532742+khanhtc1202@users.noreply.github.com> Co-authored-by: Khanh Tran <32532742+khanhtc1202@users.noreply.github.com> * Add update contributions command (#4114) * Add update contributions command * Update hack/gen-contributions.sh * Run update go deps (#4117) * feature: cli and grpc to disable application (#4119) * Add new line in detailsFormat to fix plan preview format (#4122) * add document how to disable (#4123) Co-authored-by: Kurochan <kuro@kurochan.org> Co-authored-by: Tomoki Hori <50762864+TonkyH@users.noreply.github.com> Co-authored-by: Khanh Tran <32532742+khanhtc1202@users.noreply.github.com> Co-authored-by: kevin55156 <68955641+kevin55156@users.noreply.github.com> Co-authored-by: Yoshiaki Ishihara <39365493+yoiki@users.noreply.github.com>

kurochan requested review from nghialv, khanhtc1202 and knanao as code owners November 30, 2022 17:51

github-actions bot added the area/go label Nov 30, 2022

khanhtc1202 reviewed Dec 1, 2022

View reviewed changes

khanhtc1202 reviewed Dec 5, 2022

View reviewed changes

pkg/yamlprocessor/yamlprocessor.go Outdated Show resolved Hide resolved

kurochan force-pushed the keep-new-line-at-eof branch from fabf67e to 092ba53 Compare December 8, 2022 18:06

kurochan commented Dec 8, 2022

View reviewed changes

kurochan requested a review from khanhtc1202 December 9, 2022 04:03

kurochan force-pushed the keep-new-line-at-eof branch from 092ba53 to 5cf9831 Compare December 9, 2022 06:27

kurochan force-pushed the keep-new-line-at-eof branch from 5cf9831 to 040f9e2 Compare December 22, 2022 14:44

kurochan force-pushed the keep-new-line-at-eof branch 2 times, most recently from 7cf5204 to 10b776b Compare December 31, 2022 11:13

kurochan added 9 commits January 5, 2023 18:01

Keep new line at end of file in yamlprocessor

df37b89

add a new line to the EOF in case of there is no at EOF

c06eefc

update comment

258c5a3

update eventwatcher_test

8067fdb

consider new line at EOF on parsing kubernetes manifest

0115dad

update go-yaml v1.9.8

cc9bdc5

remove TODO because PR of go-yaml was merged

89e8925

consider to handle multiple new line at EOF

c012fe0

remove indent vioration of tab

d0cae68

kurochan force-pushed the keep-new-line-at-eof branch from 10b776b to d0cae68 Compare January 5, 2023 09:01

khanhtc1202 approved these changes Jan 10, 2023

View reviewed changes

knanao approved these changes Jan 10, 2023

View reviewed changes

knanao merged commit d0bf92e into pipe-cd:master Jan 10, 2023

github-actions bot mentioned this pull request Jan 12, 2023

Release version v0.41.3 #4125

Merged

knanao mentioned this pull request Jan 13, 2023

Cherry-pick #4033 #4084 #4097 #4102 #4104 #4114 #4117 #4119 #4122 #4123 #4128

Merged

This was referenced Feb 7, 2023

Release v0.41.4 #4161

Merged

Release v0.42.0 #4162

Closed

github-actions bot mentioned this pull request Feb 27, 2023

Release v0.42.0 #4226

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep new line at end of file in yamlprocessor #4033

Keep new line at end of file in yamlprocessor #4033

kurochan commented Nov 30, 2022 •

edited

Loading

khanhtc1202 Dec 1, 2022

kurochan Dec 5, 2022

khanhtc1202 Dec 5, 2022

kurochan Dec 5, 2022

khanhtc1202 Dec 5, 2022

kurochan Dec 5, 2022 •

edited

Loading

khanhtc1202 Dec 5, 2022

kurochan Dec 5, 2022

kurochan Dec 5, 2022

khanhtc1202 commented Dec 5, 2022

kurochan commented Dec 8, 2022

khanhtc1202 commented Dec 8, 2022

kurochan Dec 8, 2022

khanhtc1202 Dec 28, 2022

kurochan Dec 31, 2022

khanhtc1202 left a comment

Keep new line at end of file in yamlprocessor #4033

Keep new line at end of file in yamlprocessor #4033

Conversation

kurochan commented Nov 30, 2022 • edited Loading

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

kurochan Dec 5, 2022 • edited Loading

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

khanhtc1202 commented Dec 5, 2022

kurochan commented Dec 8, 2022

khanhtc1202 commented Dec 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khanhtc1202 left a comment

Choose a reason for hiding this comment

kurochan commented Nov 30, 2022 •

edited

Loading

kurochan Dec 5, 2022 •

edited

Loading