-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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.
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
Refactor mapping node traversal and optimize RNode.GetAnnotations and RNode.GetLabels #4944
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -242,11 +242,7 @@ func (rn *RNode) IsTaggedNull() bool { | |
// IsNilOrEmpty is true if the node is nil, | ||
// has no YNode, or has YNode that appears empty. | ||
func (rn *RNode) IsNilOrEmpty() bool { | ||
return rn.IsNil() || | ||
IsYNodeTaggedNull(rn.YNode()) || | ||
IsYNodeEmptyMap(rn.YNode()) || | ||
IsYNodeEmptySeq(rn.YNode()) || | ||
IsYNodeZero(rn.YNode()) | ||
return rn.IsNil() || IsYNodeNilOrEmpty(rn.YNode()) | ||
} | ||
|
||
// IsStringValue is true if the RNode is not nil and is scalar string node | ||
|
@@ -420,12 +416,11 @@ func (rn *RNode) SetApiVersion(av string) { | |
// given field, so this function cannot be used to make distinctions | ||
// between these cases. | ||
func (rn *RNode) getMapFieldValue(field string) *yaml.Node { | ||
for i := 0; i < len(rn.Content()); i = IncrementFieldIndex(i) { | ||
if rn.Content()[i].Value == field { | ||
return rn.Content()[i+1] | ||
} | ||
} | ||
return nil | ||
var result *yaml.Node | ||
visitMappingNodeFields(rn.Content(), func(key, value *yaml.Node) { | ||
result = value | ||
}, field) | ||
return result | ||
} | ||
|
||
// GetName returns the name, or empty string if | ||
|
@@ -696,9 +691,9 @@ func (rn *RNode) Fields() ([]string, error) { | |
return nil, errors.Wrap(err) | ||
} | ||
var fields []string | ||
for i := 0; i < len(rn.Content()); i += 2 { | ||
fields = append(fields, rn.Content()[i].Value) | ||
} | ||
visitMappingNodeFields(rn.Content(), func(key, value *yaml.Node) { | ||
fields = append(fields, key.Value) | ||
}) | ||
return fields, nil | ||
} | ||
|
||
|
@@ -709,13 +704,12 @@ func (rn *RNode) FieldRNodes() ([]*RNode, error) { | |
return nil, errors.Wrap(err) | ||
} | ||
var fields []*RNode | ||
for i := 0; i < len(rn.Content()); i += 2 { | ||
yNode := rn.Content()[i] | ||
visitMappingNodeFields(rn.Content(), func(key, value *yaml.Node) { | ||
// for each key node in the input mapping node contents create equivalent rNode | ||
rNode := &RNode{} | ||
rNode.SetYNode(yNode) | ||
rNode.SetYNode(key) | ||
fields = append(fields, rNode) | ||
} | ||
}) | ||
return fields, nil | ||
} | ||
|
||
|
@@ -725,13 +719,11 @@ func (rn *RNode) Field(field string) *MapNode { | |
if rn.YNode().Kind != yaml.MappingNode { | ||
return nil | ||
} | ||
for i := 0; i < len(rn.Content()); i = IncrementFieldIndex(i) { | ||
isMatchingField := rn.Content()[i].Value == field | ||
if isMatchingField { | ||
return &MapNode{Key: NewRNode(rn.Content()[i]), Value: NewRNode(rn.Content()[i+1])} | ||
} | ||
} | ||
return nil | ||
var result *MapNode | ||
visitMappingNodeFields(rn.Content(), func(key, value *yaml.Node) { | ||
result = &MapNode{Key: NewRNode(key), Value: NewRNode(value)} | ||
}, field) | ||
return result | ||
} | ||
|
||
// VisitFields calls fn for each field in the RNode. | ||
|
@@ -752,6 +744,47 @@ func (rn *RNode) VisitFields(fn func(node *MapNode) error) error { | |
return nil | ||
} | ||
|
||
// visitMappingNodeFields calls fn for fields in the content, in content order. | ||
// The caller is responsible to ensure the node is a mapping node. If fieldNames | ||
// are specified, then fn is called only for the fields that match the given | ||
// fieldNames. fieldNames must contain unique values. | ||
func visitMappingNodeFields(content []*yaml.Node, fn func(key, value *yaml.Node), fieldNames ...string) { | ||
switch len(fieldNames) { | ||
case 0: // visit all fields | ||
visitFieldsWhileTrue(content, func(key, value *yaml.Node, _ int) bool { | ||
fn(key, value) | ||
return true | ||
}) | ||
default: // visit specified fields | ||
// assumption: fields in content have unique names | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need to add test coverage for this assumption before we make it. The kyaml Filters are part of the public kyaml api, so we do need to be a little bit careful. Could you pick one of the Filters that uses this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the feedback and discussion. Considering the current behavior, I don't think we should introduce an error condition here. Instead, I think the best approach is to create a local "to be found" map[string]bool of fieldNames and use that to determine when a given field should be added to the result map. I don't believe there's clear guidance since the current behavior does not support restricting the visited mapping node fields. However, I think we can use the current behavior for guidance. With a quick look around, I don't see any methods or functions that ensure mapping node keys are unique. Given that, I don't think it's appropriate to introduce that here. Instead, I think the uniqueness should be handled on the passed fieldNames. In the case of matching keys, getMapFromMeta's current behavior stores the value of the first key: The VisitFields loop iterates over the full list of keys (in order), using Field for processing. Field returns the first match. So if there are duplicates, then the key is processed multiple times - but each time Field will find the first match and its value will be what's placed into the getMapFromMeta map. I'm going to follow that pattern for how visitMappingNodeFields will execute. I'll have another commit coming to show what I'm thinking. |
||
found := 0 | ||
visitFieldsWhileTrue(content, func(key, value *yaml.Node, _ int) bool { | ||
if key == nil { | ||
return true | ||
} | ||
if !sliceutil.Contains(fieldNames, key.Value) { | ||
return true | ||
} | ||
fn(key, value) | ||
found++ | ||
return found < len(fieldNames) | ||
}) | ||
} | ||
} | ||
|
||
// visitFieldsWhileTrue calls fn for the fields in content, in content order, | ||
// until either fn returns false or all fields have been visited. The caller | ||
// should ensure that content is from a mapping node, or fits the same expected | ||
// pattern (consecutive key/value entries in the slice). | ||
func visitFieldsWhileTrue(content []*yaml.Node, fn func(key, value *yaml.Node, keyIndex int) bool) { | ||
for i := 0; i < len(content); i += 2 { | ||
continueVisiting := fn(content[i], content[i+1], i) | ||
if !continueVisiting { | ||
return | ||
} | ||
} | ||
} | ||
|
||
// Elements returns the list of elements in the RNode. | ||
// Returns an error for non-SequenceNodes. | ||
func (rn *RNode) Elements() ([]*RNode, error) { | ||
|
@@ -1003,17 +1036,19 @@ func findMergeValues(yn *yaml.Node) ([]*yaml.Node, error) { | |
// it fails. | ||
func getMergeTagValue(yn *yaml.Node) (*yaml.Node, error) { | ||
var result *yaml.Node | ||
for i := 0; i < len(yn.Content); i += 2 { | ||
key := yn.Content[i] | ||
value := yn.Content[i+1] | ||
var err error | ||
visitFieldsWhileTrue(yn.Content, func(key, value *yaml.Node, _ int) bool { | ||
if isMerge(key) { | ||
if result != nil { | ||
return nil, fmt.Errorf("duplicate merge key") | ||
err = fmt.Errorf("duplicate merge key") | ||
result = nil | ||
return false | ||
} | ||
result = value | ||
} | ||
} | ||
return result, nil | ||
return true | ||
}) | ||
return result, err | ||
} | ||
|
||
// removeMergeTags removes all merge tags and returns a ordered list of yaml | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I suggest changing the name of this bool to
fieldStillNotFound