Enable fn framework container patches to work with more kinds #4113
Conversation
|
@KnVerey: This PR has multiple commits, and the default merge method is: merge. 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. |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: KnVerey 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
KnVerey
changed the title
kyaml/yaml/fns.go
Outdated
| var result *RNode | ||
| var err error | ||
| for _, path := range paths { | ||
| result, err = PathGetter{Path: path}.Filter(resource) |
There was a problem hiding this comment.
Not sure if it really makes a difference, but should this use Lookup instead?
e.g. result, err = resource.Pipe(yaml.Lookup(path...))?
Just as a general comment, I think we need to find a faster way to parse the OpenAPI schema. Many of the issues filed regarding performance degradation is because we switch something hardcoded over to parsing the OpenAPI documents. |
KnVerey
force-pushed
the
kyaml_container_patch_kinds
branch
from
7f869ea
to
f59f5f4
Compare
kyaml/fn/framework/patch.go
Outdated
| @@ -168,8 +168,7 @@ func (cpt ContainerPatchTemplate) apply(matches []*yaml.RNode) error { | |||
| } | |||
|
|
|||
| for i := range matches { | |||
| // TODO(knverey): Make this work for more Kinds and expose the helper for doing so. | |||
| containers, err := matches[i].Pipe(yaml.Lookup("spec", "template", "spec", "containers")) | |||
| containers, err := yaml.LookupFirstMatch(yaml.ConventionalContainerPaths).Filter(matches[i]) | |||
There was a problem hiding this comment.
What is the motivation for changing the way the filter is invoked? I believe you could have still used matches[i].Pipe(yaml.LookupFirstMatch(yaml.ConventionalContainerPaths)) so I'm wondering if there is a reason you prefer to call Filter directly rather than using Pipe?
There was a problem hiding this comment.
LGTM other than this, but I'd like to understand the reasoning.
There was a problem hiding this comment.
No particular reason. As you noted, my helper originally wasn't a filter, so when I made it one, I fixed this call site by tacking Filter onto the end without looking back at the original diff. The only difference if you only have one Filter seems to be that Pipe short-circuits when given nil input, and special-cases root document nodes. Doesn't hurt to use it here, or in the helper. Since it raised questions for you, I'll change it.
KnVerey
force-pushed
the
kyaml_container_patch_kinds
branch
from
f59f5f4
to
75df1a5
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
Fixes a TODO in the fn framework: make container patches support more workload kinds. The perfect way to do this would be to traverse the openAPI schemas looking for embedded container. That would work for CRs too, IF we've been given schemas for them AND they use references rather than redefining the container spec. But since there is a very small set of well-known paths, this much simpler solution works equally well for the built-in workload kinds.