From 92826c6a1e91d8a04180ebc3ef50386c90f4925a Mon Sep 17 00:00:00 2001 From: Donny Xia Date: Thu, 15 Oct 2020 16:26:37 -0700 Subject: [PATCH 1/2] support array index in PathGetter --- kyaml/yaml/fns.go | 61 +++++++++++++++++++++++++++++++++++++++--- kyaml/yaml/fns_test.go | 52 +++++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 4 deletions(-) diff --git a/kyaml/yaml/fns.go b/kyaml/yaml/fns.go index 50a1980a3..dad03d32e 100644 --- a/kyaml/yaml/fns.go +++ b/kyaml/yaml/fns.go @@ -6,6 +6,7 @@ package yaml import ( "fmt" "regexp" + "strconv" "strings" "github.com/davecgh/go-spew/spew" @@ -123,6 +124,41 @@ func (e ElementSetter) Filter(rn *RNode) (*RNode, error) { return NewRNode(e.Element), nil } +// GetElementByIndex will return a Filter which can be applied to a sequence +// node to get the element specified by the index +func GetElementByIndex(index string) ElementPicker { + return ElementPicker{Index: index} +} + +// ElementPicker picks the element with a specified index. Index starts from +// 0 to len(list) - 1. a hyphen ("-") means the last index. +type ElementPicker struct { + Index string +} + +// Filter implements Filter +func (p ElementPicker) Filter(rn *RNode) (*RNode, error) { + lastElement := false + idx, err := strconv.Atoi(p.Index) + if err != nil { + if p.Index != "-" { + return nil, errors.Errorf("unknown index %s", p.Index) + } + lastElement = true + } + elems, err := rn.Elements() + if err != nil { + return nil, err + } + if lastElement { + return elems[len(elems)-1], nil + } + if idx < 0 || idx >= len(elems) { + return nil, nil + } + return elems[idx], nil +} + // Clear returns a FieldClearer func Clear(name string) FieldClearer { return FieldClearer{Name: name} @@ -364,12 +400,14 @@ type PathGetter struct { // Each path part may be one of: // * FieldMatcher -- e.g. "spec" // * Map Key -- e.g. "app.k8s.io/version" - // * List Entry -- e.g. "[name=nginx]" or "[=-jar]" + // * List Entry -- e.g. "[name=nginx]" or "[=-jar]" or "0" // // Map Keys and Fields are equivalent. // See FieldMatcher for more on Fields and Map Keys. // - // List Entries are specified as map entry to match [fieldName=fieldValue]. + // List Entries can be specified as map entry to match [fieldName=fieldValue] + // or a postional index like 0 to get the element. - is special and means the + // last element. // See Elem for more on List Entries. // // Examples: @@ -382,6 +420,8 @@ type PathGetter struct { // * The leaf Node (final path) will be created with a Kind matching Create // * Intermediary Nodes will be created as either a MappingNodes or // SequenceNodes as appropriate for each's Path location. + // * If a list item is specified by a index (an offset or "-"), this item will + // not be created even Create is set. Create yaml.Kind `yaml:"create,omitempty"` // Style is the style to apply to created value Nodes. @@ -402,9 +442,12 @@ func (l PathGetter) Filter(rn *RNode) (*RNode, error) { if len(l.Path) > i+1 { nextPart = l.Path[i+1] } - if IsListIndex(part) { + switch { + case IsListIndex(part): match, err = l.doElem(match, part) - } else { + case isPositionalIndex(part): + match, err = match.Pipe(GetElementByIndex(part)) + default: fieldPath = append(fieldPath, part) match, err = l.doField(match, part, l.getKind(nextPart)) } @@ -641,6 +684,16 @@ func IsListIndex(p string) bool { return strings.HasPrefix(p, "[") && strings.HasSuffix(p, "]") } +// isPositionalIndex returns true if the index is a positional +// index like 0, 1..., or - (last element) +func isPositionalIndex(index string) bool { + if index == "-" { + return true + } + _, err := strconv.Atoi(index) + return err == nil +} + // SplitIndexNameValue splits a lookup part Val index into the field name // and field value to match. // e.g. splits [name=nginx] into (name, nginx) diff --git a/kyaml/yaml/fns_test.go b/kyaml/yaml/fns_test.go index 1b4e327c5..51bcec113 100644 --- a/kyaml/yaml/fns_test.go +++ b/kyaml/yaml/fns_test.go @@ -57,6 +57,36 @@ func TestAppend(t *testing.T) { assert.Nil(t, rn) } +func TestGetElementByIndex(t *testing.T) { + node, err := Parse(` +- 0 +- 1 +- 2 +`) + assert.NoError(t, err) + + rn, err := node.Pipe(GetElementByIndex("0")) + assert.NoError(t, err) + assert.Equal(t, "0\n", assertNoErrorString(t)(rn.String())) + + rn, err = node.Pipe(GetElementByIndex("2")) + assert.NoError(t, err) + assert.Equal(t, "2\n", assertNoErrorString(t)(rn.String())) + + rn, err = node.Pipe(GetElementByIndex("-")) + assert.NoError(t, err) + assert.Equal(t, "2\n", assertNoErrorString(t)(rn.String())) + + _, err = node.Pipe(GetElementByIndex("#")) + assert.Errorf(t, err, "unknown index #") + + rn, _ = node.Pipe(GetElementByIndex("-1")) + assert.Nil(t, rn) + + rn, _ = node.Pipe(GetElementByIndex("4")) + assert.Nil(t, rn) +} + func TestGetElementByKey(t *testing.T) { node, err := Parse(` - b: c @@ -340,6 +370,18 @@ j: k p: q `, assertNoErrorString(t)(rn.String())) + rn, err = node.Pipe(Lookup("a", "b", "0")) + assert.NoError(t, err) + assert.Equal(t, s, assertNoErrorString(t)(node.String())) + assert.Equal(t, `f: g +`, assertNoErrorString(t)(rn.String())) + + rn, err = node.Pipe(Lookup("a", "b", "-", "h")) + assert.NoError(t, err) + assert.Equal(t, s, assertNoErrorString(t)(node.String())) + assert.Equal(t, `i +`, assertNoErrorString(t)(rn.String())) + rn, err = node.Pipe(Lookup("l")) assert.NoError(t, err) assert.Equal(t, s, assertNoErrorString(t)(node.String())) @@ -384,6 +426,16 @@ j: k assert.NoError(t, err) assert.Equal(t, s, assertNoErrorString(t)(node.String())) assert.Nil(t, rn) + + rn, err = node.Pipe(Lookup("a", "b", "-1")) + assert.NoError(t, err) + assert.Equal(t, s, assertNoErrorString(t)(node.String())) + assert.Nil(t, rn) + + rn, err = node.Pipe(Lookup("a", "b", "99")) + assert.NoError(t, err) + assert.Equal(t, s, assertNoErrorString(t)(node.String())) + assert.Nil(t, rn) } func TestSetField_Fn(t *testing.T) { From 9c7b4fddf9033571e7aa791b478862bc14f0940e Mon Sep 17 00:00:00 2001 From: Donny Xia Date: Tue, 20 Oct 2020 12:39:50 -0700 Subject: [PATCH 2/2] code review --- kyaml/yaml/fns.go | 93 ++++++++++++++++++++++-------------------- kyaml/yaml/fns_test.go | 17 ++------ 2 files changed, 52 insertions(+), 58 deletions(-) diff --git a/kyaml/yaml/fns.go b/kyaml/yaml/fns.go index dad03d32e..45b107618 100644 --- a/kyaml/yaml/fns.go +++ b/kyaml/yaml/fns.go @@ -126,37 +126,30 @@ func (e ElementSetter) Filter(rn *RNode) (*RNode, error) { // GetElementByIndex will return a Filter which can be applied to a sequence // node to get the element specified by the index -func GetElementByIndex(index string) ElementPicker { - return ElementPicker{Index: index} +func GetElementByIndex(index int) ElementIndexer { + return ElementIndexer{Index: index} } -// ElementPicker picks the element with a specified index. Index starts from +// ElementIndexer picks the element with a specified index. Index starts from // 0 to len(list) - 1. a hyphen ("-") means the last index. -type ElementPicker struct { - Index string +type ElementIndexer struct { + Index int } // Filter implements Filter -func (p ElementPicker) Filter(rn *RNode) (*RNode, error) { - lastElement := false - idx, err := strconv.Atoi(p.Index) - if err != nil { - if p.Index != "-" { - return nil, errors.Errorf("unknown index %s", p.Index) - } - lastElement = true - } +func (i ElementIndexer) Filter(rn *RNode) (*RNode, error) { + // rn.Elements will return error if rn is not a sequence node. elems, err := rn.Elements() if err != nil { return nil, err } - if lastElement { + if i.Index < 0 { return elems[len(elems)-1], nil } - if idx < 0 || idx >= len(elems) { + if i.Index >= len(elems) { return nil, nil } - return elems[idx], nil + return elems[i.Index], nil } // Clear returns a FieldClearer @@ -400,14 +393,15 @@ type PathGetter struct { // Each path part may be one of: // * FieldMatcher -- e.g. "spec" // * Map Key -- e.g. "app.k8s.io/version" - // * List Entry -- e.g. "[name=nginx]" or "[=-jar]" or "0" + // * List Entry -- e.g. "[name=nginx]" or "[=-jar]" or "0" or "-" // // Map Keys and Fields are equivalent. // See FieldMatcher for more on Fields and Map Keys. // // List Entries can be specified as map entry to match [fieldName=fieldValue] - // or a postional index like 0 to get the element. - is special and means the - // last element. + // or a postional index like 0 to get the element. - (unquoted hyphen) is + // special and means the last element. + // // See Elem for more on List Entries. // // Examples: @@ -442,15 +436,12 @@ func (l PathGetter) Filter(rn *RNode) (*RNode, error) { if len(l.Path) > i+1 { nextPart = l.Path[i+1] } - switch { - case IsListIndex(part): - match, err = l.doElem(match, part) - case isPositionalIndex(part): - match, err = match.Pipe(GetElementByIndex(part)) - default: - fieldPath = append(fieldPath, part) - match, err = l.doField(match, part, l.getKind(nextPart)) + var fltr Filter + fltr, err = l.getFilter(part, nextPart, &fieldPath) + if err != nil { + return nil, err } + match, err = match.Pipe(fltr) if IsMissingOrError(match, err) { return nil, err } @@ -459,14 +450,36 @@ func (l PathGetter) Filter(rn *RNode) (*RNode, error) { return match, nil } -func (l PathGetter) doElem(rn *RNode, part string) (*RNode, error) { +func (l PathGetter) getFilter(part, nextPart string, fieldPath *[]string) (Filter, error) { + idx, err := strconv.Atoi(part) + switch { + case err == nil: + // part is a number + if idx < 0 { + return nil, fmt.Errorf("array index %d cannot be negative", idx) + } + return GetElementByIndex(idx), nil + case part == "-": + // part is a hyphen + return GetElementByIndex(-1), nil + case IsListIndex(part): + // part is surrounded by brackets + return l.elemFilter(part) + default: + // mapping node + *fieldPath = append(*fieldPath, part) + return l.fieldFilter(part, l.getKind(nextPart)) + } +} + +func (l PathGetter) elemFilter(part string) (Filter, error) { var match *RNode name, value, err := SplitIndexNameValue(part) if err != nil { return nil, errors.Wrap(err) } if !IsCreate(l.Create) { - return rn.Pipe(MatchElement(name, value)) + return MatchElement(name, value), nil } var elem *RNode @@ -486,15 +499,15 @@ func (l PathGetter) doElem(rn *RNode, part string) (*RNode, error) { }) } // Append the Node - return rn.Pipe(ElementMatcher{FieldName: name, FieldValue: value, Create: elem}) + return ElementMatcher{FieldName: name, FieldValue: value, Create: elem}, nil } -func (l PathGetter) doField( - rn *RNode, name string, kind yaml.Kind) (*RNode, error) { +func (l PathGetter) fieldFilter( + name string, kind yaml.Kind) (Filter, error) { if !IsCreate(l.Create) { - return rn.Pipe(Get(name)) + return Get(name), nil } - return rn.Pipe(FieldMatcher{Name: name, Create: &RNode{value: &yaml.Node{Kind: kind, Style: l.Style}}}) + return FieldMatcher{Name: name, Create: &RNode{value: &yaml.Node{Kind: kind, Style: l.Style}}}, nil } func (l PathGetter) getKind(nextPart string) yaml.Kind { @@ -684,16 +697,6 @@ func IsListIndex(p string) bool { return strings.HasPrefix(p, "[") && strings.HasSuffix(p, "]") } -// isPositionalIndex returns true if the index is a positional -// index like 0, 1..., or - (last element) -func isPositionalIndex(index string) bool { - if index == "-" { - return true - } - _, err := strconv.Atoi(index) - return err == nil -} - // SplitIndexNameValue splits a lookup part Val index into the field name // and field value to match. // e.g. splits [name=nginx] into (name, nginx) diff --git a/kyaml/yaml/fns_test.go b/kyaml/yaml/fns_test.go index 51bcec113..e7dda5f71 100644 --- a/kyaml/yaml/fns_test.go +++ b/kyaml/yaml/fns_test.go @@ -65,26 +65,17 @@ func TestGetElementByIndex(t *testing.T) { `) assert.NoError(t, err) - rn, err := node.Pipe(GetElementByIndex("0")) + rn, err := node.Pipe(GetElementByIndex(0)) assert.NoError(t, err) assert.Equal(t, "0\n", assertNoErrorString(t)(rn.String())) - rn, err = node.Pipe(GetElementByIndex("2")) + rn, err = node.Pipe(GetElementByIndex(2)) assert.NoError(t, err) assert.Equal(t, "2\n", assertNoErrorString(t)(rn.String())) - rn, err = node.Pipe(GetElementByIndex("-")) + rn, err = node.Pipe(GetElementByIndex(-1)) assert.NoError(t, err) assert.Equal(t, "2\n", assertNoErrorString(t)(rn.String())) - - _, err = node.Pipe(GetElementByIndex("#")) - assert.Errorf(t, err, "unknown index #") - - rn, _ = node.Pipe(GetElementByIndex("-1")) - assert.Nil(t, rn) - - rn, _ = node.Pipe(GetElementByIndex("4")) - assert.Nil(t, rn) } func TestGetElementByKey(t *testing.T) { @@ -428,7 +419,7 @@ j: k assert.Nil(t, rn) rn, err = node.Pipe(Lookup("a", "b", "-1")) - assert.NoError(t, err) + assert.Errorf(t, err, "array index -1 cannot be negative") assert.Equal(t, s, assertNoErrorString(t)(node.String())) assert.Nil(t, rn)