Make seq indent configurable and add retain seq indent functionality #4043

phanimarupaka · 2021-07-07T22:39:56Z

Make sequenceIndentationStyle in alias.go configurable by adding getter and setter
Add optional functionality to retain SeqIndent. This is purely an additional change and doesn't affect any of the existing tests. The motivation is to make kio package expose an option to downstream callers(like kpt) to retain the existing indentation of resources. This is done by adding a new internal annotation with name internal.config.kubernetes.io/seqindent. The byteio_reader adds this annotation so that byteio_write can encode the resources based on the indentation specified in the annotation.

Adding more tests. But want to confirm the direction.

@droot @KnVerey @natasha41575

kyaml/kio/byteio_writer.go

kyaml/yaml/alias.go

kyaml/kio/pkgio_reader.go

kyaml/kio/kioutil/kioutil.go

kyaml/kio/byteio_reader.go

k8s-ci-robot · 2021-07-08T06:22:32Z

@phanimarupaka: This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

droot

Looking better. Some more thoughts.

kyaml/kio/byteio_reader.go

kyaml/yaml/alias.go

phanimarupaka · 2021-07-09T00:35:57Z

@KnVerey @natasha41575 Let us know your thoughts.

kyaml/kio/byteio_reader_test.go

kyaml/kio/byteio_readwriter_test.go

kyaml/yaml/alias.go

KnVerey · 2021-07-09T00:39:50Z

kyaml/kio/byteio_reader.go

@@ -37,6 +38,9 @@ type ByteReadWriter struct {
 	// the Resources, otherwise they will be cleared.
 	KeepReaderAnnotations bool

+	// AddSeqIndentAnnotation if true adds kioutil.SeqIndentAnnotation to each resource
+	AddSeqIndentAnnotation bool


Would it make sense to name this for the effect rather than the mechanism? E.g. something like PreserveSequenceIndentStyle

There's also an awkward conflict between this option (however named) and OmitReaderAnnotations. If we can do it cheaply enough, wdyt about treating it like any other reader annotation, and adding a "preserve indent style" option on the writer to make use of it where available?

Related to this, I see that the writer already has a Style option, which I'm guessing might also interact with this new feature. E.g. if the Style is set to the json-like FlowStyle where sequence indent style doesn't apply, I'm guessing Style wins. We should document/test this.

There's also an awkward conflict between this option (however named) and OmitReaderAnnotations. If we can do it cheaply enough, wdyt about treating it like any other reader annotation, and adding a "preserve indent style" option on the writer to make use of it where available?

It is not cheap to identify the indentation in byteio_reader. Hence made it opt-in. Except for that, this annotation is identical to index annotation and follows the same life cycle.

Related to this, I see that the writer already has a Style option, which I'm guessing might also interact with this new feature. E.g. if the Style is set to the json-like FlowStyle where sequence indent style doesn't apply, I'm guessing Style wins. We should document/test this.

That is a good point. Few options clash here.

Both OmitReaderAnnotations and Style win over PreserveSequenceIndentStyle. I will document and test this behavior.

Would it make sense to name this for the effect rather than the mechanism? E.g. something like PreserveSequenceIndentStyle

I started off with this but @droot thinks otherwise. I was on the fence. But the more I think, I think it is better to go with the mechanism rather than annotation(@droot WDYT). In that case, should we error out if conflicting options are set ? For e.g. OmitReaderAnnotations and PreserveSequenceIndentStyle are set ?

Yes, PreserveSequenceIndentStyle makes sense at the ReaderWriter level or PackageReaderWriter level because they do reading and writing so can encapsulate the preserving behavior. So +1 to that.
The same name may not make sense at the Reader level because reader can't ensure that behavior, so mechanism fits better there.

Re: OmitReaderAnnotations. If we were starting today, would have definitely made the preserveIdentStyle as the default behavior and treating it like ReaderAnnotation. But at this point, to reduce the blast radius (and the cost involved), making this additive makes sense. There is an alternative path here, where we introduce this as an opt-in now (keeping it consistent with ReaderAnnotation) and eventually make it the default behavior.

ByteReader is also exposed. We may change the logic even at bytereader as well. So it is better to be consistent.

KnVerey · 2021-07-09T01:09:25Z

kyaml/kio/byteio_reader.go

+	noIndentDiff := copyutil.PrettyFileDiff(string(out), originalYAML)
+
+	var style string
+	if len(noIndentDiff) <= len(twoSpaceIndentDiff) {


Will this heuristic be confounded by a non-2 general indent level with compact style? Rather than attempting to determine the average indent level, could we sample the first sequence somehow (scanning? column data on YNodes?) and use that? I imagine most documents would be consistent, and when they're not, "first" behaviour might be easier for the end user to understand than "average".

From the feedback we received, users want to see less diffs. The goal here is to minimize the diff. So the "first" behavior doesn't achieve that goal.

And identifying first sequence is difficult problem. There would be many edge cases.

Not sure on the complexity of identify the "first" behavior, but picking the "first" behavior has more benefits over the "average" when indentation is not consistent within a resource. It makes it more deterministic and can potentially make this function cheaper as well. We should definitely explore if it is a possibility.

I did a benchmark analysis and found that the latency is 2.5 times more. Implemented "first" approach. There is no significant performance degradation. Since the new approach involves raw YAML string parsing, I might be missing few corner cases. We can improve it as we go.

kyaml/kio/byteio_reader.go

droot · 2021-07-09T18:34:29Z

kyaml/yaml/alias.go

-const DefaultIndent = 2
-const DefaultSequenceStyle = CompactSequenceStyle
+// SeqIndentType holds the indentation style for sequence nodes
+type SeqIndentType string


nit: SeqIndentStyle represent the behavior better. Also, preserve the older CompactSequenceStyle, WideSequenceStyle names to reduce the changes on public symbols.

droot · 2021-07-09T18:40:21Z

kyaml/kio/byteio_reader.go

@@ -37,6 +38,9 @@ type ByteReadWriter struct {
 	// the Resources, otherwise they will be cleared.
 	KeepReaderAnnotations bool

+	// AddSeqIndentAnnotation if true adds kioutil.SeqIndentAnnotation to each resource
+	AddSeqIndentAnnotation bool


Yes, PreserveSequenceIndentStyle makes sense at the ReaderWriter level or PackageReaderWriter level because they do reading and writing so can encapsulate the preserving behavior. So +1 to that.
The same name may not make sense at the Reader level because reader can't ensure that behavior, so mechanism fits better there.

droot

@phanimarupaka The change looks good. I have minor comments.

kyaml/kio/byteio_reader_test.go

kyaml/kio/byteio_readwriter_test.go

kyaml/yaml/util_test.go

droot · 2021-07-13T17:22:32Z

/lgtm

k8s-ci-robot · 2021-07-13T17:22:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: droot, phanimarupaka

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [phanimarupaka]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 7, 2021

k8s-ci-robot requested review from mortent and natasha41575 July 7, 2021 22:40

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 7, 2021

natasha41575 linked an issue Jul 7, 2021 that may be closed by this pull request

Make kyaml indentation style configurable #4029

Closed

phanimarupaka force-pushed the PreserveIndentation branch from 936c22f to 86f1cdc Compare July 7, 2021 22:55

Make seq indent configurable and add retain seq indent functionality

ed38b5f

phanimarupaka force-pushed the PreserveIndentation branch from 86f1cdc to ed38b5f Compare July 7, 2021 23:53

droot suggested changes Jul 8, 2021

View reviewed changes

Suggested changes

6dbc74b

droot reviewed Jul 8, 2021

View reviewed changes

kyaml/kio/byteio_reader.go Outdated Show resolved Hide resolved

kyaml/kio/byteio_reader.go Outdated Show resolved Hide resolved

kyaml/yaml/alias.go Outdated Show resolved Hide resolved

phanimarupaka force-pushed the PreserveIndentation branch 3 times, most recently from a2814b7 to 2a99e0b Compare July 8, 2021 21:49

Add options, keep seqindent annotation equivalent to index annotation

f81201b

phanimarupaka force-pushed the PreserveIndentation branch from 2a99e0b to f81201b Compare July 8, 2021 21:54

phanimarupaka mentioned this pull request Jul 9, 2021

Remove default formatting, preserve field order and indentation kptdev/kpt#2376

Merged

KnVerey reviewed Jul 9, 2021

View reviewed changes

phanimarupaka added 2 commits July 9, 2021 00:17

Update comments, tests, not expose indent option

c07ffa5

Change annotation name, error if conflicting options

89b12cf

phanimarupaka requested a review from KnVerey July 9, 2021 18:27

droot reviewed Jul 9, 2021

View reviewed changes

First sequence indent wins

74e8678

droot reviewed Jul 12, 2021

View reviewed changes

phanimarupaka force-pushed the PreserveIndentation branch from b86d0a6 to b6fb62b Compare July 13, 2021 06:32

Suggested changes

29be7fa

phanimarupaka force-pushed the PreserveIndentation branch from b6fb62b to 29be7fa Compare July 13, 2021 06:34

phanimarupaka requested a review from droot July 13, 2021 06:34

droot approved these changes Jul 13, 2021

View reviewed changes

k8s-ci-robot assigned droot Jul 13, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 13, 2021

k8s-ci-robot merged commit 4a13725 into kubernetes-sigs:master Jul 13, 2021

KnVerey mentioned this pull request Aug 4, 2021

Let kyaml keep white space formatting #3559

Closed

erikgb mentioned this pull request May 24, 2022

Instruct kyaml/kio to retain sequence indentation style fluxcd/image-automation-controller#366

Merged

sa-spag mentioned this pull request Dec 8, 2022

Retain YAML formatting when using the Git write-back method argoproj-labs/argocd-image-updater#501

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make seq indent configurable and add retain seq indent functionality #4043

Make seq indent configurable and add retain seq indent functionality #4043

phanimarupaka commented Jul 7, 2021 •

edited

Loading

k8s-ci-robot commented Jul 8, 2021

droot left a comment

phanimarupaka commented Jul 9, 2021

KnVerey Jul 9, 2021

KnVerey Jul 9, 2021

phanimarupaka Jul 9, 2021

phanimarupaka Jul 9, 2021

droot Jul 9, 2021

droot Jul 9, 2021

phanimarupaka Jul 9, 2021

KnVerey Jul 9, 2021

phanimarupaka Jul 9, 2021

droot Jul 9, 2021

phanimarupaka Jul 9, 2021

droot Jul 9, 2021

droot Jul 9, 2021

droot left a comment

droot commented Jul 13, 2021

k8s-ci-robot commented Jul 13, 2021

Make seq indent configurable and add retain seq indent functionality #4043

Make seq indent configurable and add retain seq indent functionality #4043

Conversation

phanimarupaka commented Jul 7, 2021 • edited Loading

k8s-ci-robot commented Jul 8, 2021

droot left a comment

Choose a reason for hiding this comment

phanimarupaka commented Jul 9, 2021

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

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

droot left a comment

Choose a reason for hiding this comment

droot commented Jul 13, 2021

k8s-ci-robot commented Jul 13, 2021

phanimarupaka commented Jul 7, 2021 •

edited

Loading