Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Jul 13, 2021
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ed38b5f
Make seq indent configurable and add retain seq indent functionality
phanimarupaka Jul 7, 2021
6dbc74b
Suggested changes
phanimarupaka Jul 8, 2021
f81201b
Add options, keep seqindent annotation equivalent to index annotation
phanimarupaka Jul 8, 2021
c07ffa5
Update comments, tests, not expose indent option
phanimarupaka Jul 9, 2021
89b12cf
Change annotation name, error if conflicting options
phanimarupaka Jul 9, 2021
74e8678
First sequence indent wins
phanimarupaka Jul 9, 2021
29be7fa
Suggested changes
phanimarupaka Jul 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 55 additions & 2 deletions kyaml/kio/byteio_reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"sort"
"strings"

"sigs.k8s.io/kustomize/kyaml/copyutil"
"sigs.k8s.io/kustomize/kyaml/errors"
"sigs.k8s.io/kustomize/kyaml/kio/kioutil"
"sigs.k8s.io/kustomize/kyaml/yaml"
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Style is a style that is set on the Resource Node Document.
Style yaml.Style

Expand All @@ -51,8 +55,9 @@ type ByteReadWriter struct {

func (rw *ByteReadWriter) Read() ([]*yaml.RNode, error) {
b := &ByteReader{
Reader: rw.Reader,
OmitReaderAnnotations: rw.OmitReaderAnnotations,
Reader: rw.Reader,
OmitReaderAnnotations: rw.OmitReaderAnnotations,
AddSeqIndentAnnotation: rw.AddSeqIndentAnnotation,
}
val, err := b.Read()
if rw.FunctionConfig == nil {
Expand Down Expand Up @@ -112,6 +117,9 @@ type ByteReader struct {
// annotation on Resources as they are Read.
OmitReaderAnnotations bool

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

// SetAnnotations is a map of caller specified annotations to set on resources as they are read
// These are independent of the annotations controlled by OmitReaderAnnotations
SetAnnotations map[string]string
Expand Down Expand Up @@ -195,6 +203,13 @@ func (r *ByteReader) Read() ([]*yaml.RNode, error) {
if err == io.EOF {
continue
}

if r.AddSeqIndentAnnotation {
if err := addSeqIndentAnno(values[i], node); err != nil {
return nil, errors.Wrap(err)
}
}

if err != nil {
return nil, errors.Wrap(err)
}
Expand Down Expand Up @@ -245,6 +260,44 @@ func (r *ByteReader) Read() ([]*yaml.RNode, error) {
return output, nil
}

// addSeqIndentAnno adds the sequence indentation annotation to the resource
// value is the input yaml string and node is the decoded equivalent of value
// the annotation value is decided by deriving the existing sequence indentation of resource
func addSeqIndentAnno(value string, node *yaml.RNode) error {
phanimarupaka marked this conversation as resolved.
Show resolved Hide resolved
// step 1: derive the sequence indentation of the node
anno := node.GetAnnotations()
if anno[kioutil.SeqIndentAnnotation] != "" {
// the annotation already exists, so don't change it
return nil
}

// marshal the node with 2 space sequence indentation and calculate the diff
out, err := yaml.MarshalWithIndent(node.Document(), yaml.DefaultMapIndent, yaml.WideSeqIndent)
if err != nil {
return err
}
twoSpaceIndentDiff := copyutil.PrettyFileDiff(string(out), value)

// marshal the node with 0 space sequence indentation and calculate the diff
out, err = yaml.MarshalWithIndent(node.Document(), yaml.DefaultMapIndent, yaml.CompactSeqIndent)
if err != nil {
return err
}
noIndentDiff := copyutil.PrettyFileDiff(string(out), value)

var style string
if len(noIndentDiff) <= len(twoSpaceIndentDiff) {
style = string(yaml.CompactSeqIndent)
} else {
style = string(yaml.WideSeqIndent)
}
phanimarupaka marked this conversation as resolved.
Show resolved Hide resolved

// step 2: add the annotation to the node
return node.PipeE(
yaml.LookupCreate(yaml.MappingNode, yaml.MetadataField, yaml.AnnotationsField),
yaml.SetField(kioutil.SeqIndentAnnotation, yaml.NewScalarRNode(style)))
}

func (r *ByteReader) decode(index int, decoder *yaml.Decoder) (*yaml.RNode, error) {
node := &yaml.Node{}
err := decoder.Decode(node)
Expand Down
119 changes: 119 additions & 0 deletions kyaml/kio/byteio_reader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -805,3 +805,122 @@ items:
})
}
}

func TestByteReader_AddSeqIndent(t *testing.T) {
type testCase struct {
name string
err string
input string
expectedOutput string
}

testCases := []testCase{
{
name: "read with wide indentation",
input: `apiVersion: apps/v1
kind: Deployment
spec:
- foo
- bar
- baz
`,
expectedOutput: `apiVersion: apps/v1
kind: Deployment
spec:
- foo
- bar
- baz
metadata:
annotations:
internal.config.kubernetes.io/seqindent: wide
`,
},
{
name: "read with compact indentation",
input: `apiVersion: apps/v1
kind: Deployment
spec:
- foo
- bar
- baz
`,
expectedOutput: `apiVersion: apps/v1
kind: Deployment
spec:
- foo
- bar
- baz
metadata:
annotations:
internal.config.kubernetes.io/seqindent: compact
`,
},
{
name: "read with mixed indentation, wide wins",
input: `apiVersion: apps/v1
kind: Deployment
spec:
- foo
- bar
- baz
env:
- foo
- bar
`,
expectedOutput: `apiVersion: apps/v1
kind: Deployment
spec:
- foo
- bar
- baz
env:
- foo
- bar
metadata:
annotations:
internal.config.kubernetes.io/seqindent: wide
`,
},
{
name: "read with mixed indentation, compact wins",
input: `apiVersion: apps/v1
kind: Deployment
spec:
- foo
- bar
- baz
env:
- foo
- bar
`,
expectedOutput: `apiVersion: apps/v1
kind: Deployment
spec:
- foo
- bar
- baz
env:
- foo
- bar
metadata:
annotations:
internal.config.kubernetes.io/seqindent: compact
`,
},
}

for i := range testCases {
tc := testCases[i]
t.Run(tc.name, func(t *testing.T) {
rNodes, err := (&ByteReader{
OmitReaderAnnotations: true,
AddSeqIndentAnnotation: true,
Reader: bytes.NewBuffer([]byte(tc.input)),
}).Read()
assert.NoError(t, err)
actual, err := rNodes[0].String()
assert.NoError(t, err)
assert.Equal(t, tc.expectedOutput, actual)
})
}
}
Loading