add replacement filter to support replacmenttransformer #3735

natasha41575 · 2021-03-17T20:57:26Z

Part of #3492

The existing, untested replacement transformer converts the resource to a map, applies the transformations, and then must convert it back to a resource. Instead of doing this, it will be better to directly transform the resource's underlying RNode and avoiding these conversions. Here is an implementation for a filter to do it. For now it is only handles simple cases, we can build on it after we have a simple version working.

Next steps would be to rewrite the replacement transformer plugin to use this filter, and move the plugin to the examples folder.

api/filters/replacement/doc.go

api/filters/replacement/example_test.go

api/filters/replacement/replacement.go

KnVerey

I have a question about the feature set we want for the source/target fields, but it doesn't need to block this initial iteration from merging.

/lgtm

api/filters/replacement/replacement.go

api/types/replacement.go

natasha41575 · 2021-03-22T19:42:52Z

@monopole could you take a look? This still needs approval from someone on the kustomize maintainers list

monopole

added comment above, request no new use of the Target struct.

natasha41575 · 2021-03-23T20:38:46Z

@monopole @KnVerey updated the PR to reflect the proposed config

I also added a field (fieldPaths) to make it possible for targets to have multiple fieldpaths. An example use case for this is in one of the tests, but if this functionality is not desired I can take it out. To keep the size of the PR down, I haven't yet implemented use of the deny or options field; I plan to do that in a later PR.

api/filters/replacement/replacement.go

monopole · 2021-03-24T00:22:47Z

looks great, just minor nits. thanks for deferring Deny and fieldoptions.

api/filters/replacement/replacement_test.go

api/types/replacement.go

api/filters/replacement/replacement.go

api/types/replacement.go

api/filters/replacement/replacement.go

KnVerey

I think we should add more test coverage, including regression cases for the empty namespace and missing group issues identified in the last round. But that can be done in follow-ups; there's already at least one other testing TODO. Found a few documentation typos, but they're about the deferred fields anyway.

/lgtm

KnVerey · 2021-04-01T00:35:34Z

api/types/replacement.go

+	Select *Selector `json:"select" yaml:"select"`
+
+	// From the allowed set, remove objects that match this.
+	// TODO (#3492): Remove matches listed in the exclude field


Suggested change

// TODO (#3492): Remove matches listed in the exclude field

// TODO (#3492): Remove matches listed in the Reject field

KnVerey · 2021-04-01T00:36:50Z

api/types/replacement.go

-type ReplTarget struct {
-	ObjRef    *Selector `json:"objref,omitempty" yaml:"objref,omitempty"`
-	FieldRefs []string  `json:"fieldrefs,omitempty" yaml:"fieldrefs,omitempty"`
+// FieldPath is a structured field path to the desired object


Suggested change

// FieldPath is a structured field path to the desired object

// FieldOptions refine the interpretation of FieldPaths.

KnVerey · 2021-04-01T00:36:59Z

api/types/replacement.go

-	FieldRefs []string  `json:"fieldrefs,omitempty" yaml:"fieldrefs,omitempty"`
+// FieldPath is a structured field path to the desired object
+// TODO (#3492): Implement use of these options, they are
+//  currently used


Suggested change

// currently used

// currently unused

KnVerey · 2021-04-01T00:43:24Z

api/filters/replacement/replacement_test.go

+		input        string
+		replacements string
+		expected     string
+		expectedErr  bool


Is using a bool for this the norm on this project? I'm used to matching against an error string when an error is expected, since there are typically multiple places an error could occur, and it's also nice to assert against what the user will actually see.

I can update this on a followup PR (along with all the typos). Thanks so much for your help!

k8s-ci-robot · 2021-04-01T01:01:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KnVerey, monopole, natasha41575

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~~ [monopole]

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

natasha41575 requested review from monopole, KnVerey and Shell32-Natsu March 17, 2021 20:57

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 17, 2021

k8s-ci-robot requested a review from droot March 17, 2021 20:57

natasha41575 removed the request for review from droot March 17, 2021 20:58

natasha41575 force-pushed the ReplacementFilter branch 2 times, most recently from e7cc1a1 to 35422be Compare March 18, 2021 20:13

natasha41575 mentioned this pull request Mar 18, 2021

replacement transformer that uses the new replacement filter #3737

Merged

KnVerey reviewed Mar 18, 2021

View reviewed changes

api/filters/replacement/replacement.go Outdated Show resolved Hide resolved

natasha41575 force-pushed the ReplacementFilter branch from 35422be to 71afbb8 Compare March 19, 2021 21:04

natasha41575 requested a review from KnVerey March 19, 2021 21:10

KnVerey reviewed Mar 19, 2021

View reviewed changes

api/filters/replacement/replacement.go Show resolved Hide resolved

api/filters/replacement/replacement.go Outdated Show resolved Hide resolved

natasha41575 force-pushed the ReplacementFilter branch from 71afbb8 to dac984e Compare March 19, 2021 23:30

natasha41575 requested a review from KnVerey March 19, 2021 23:34

natasha41575 force-pushed the ReplacementFilter branch from dac984e to c6bf10f Compare March 19, 2021 23:51

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 19, 2021

KnVerey approved these changes Mar 20, 2021

View reviewed changes

api/filters/replacement/replacement.go Outdated Show resolved Hide resolved

api/types/replacement.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned KnVerey Mar 20, 2021

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

natasha41575 force-pushed the ReplacementFilter branch from c6bf10f to 14b64b1 Compare March 22, 2021 19:35

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 22, 2021

natasha41575 assigned monopole Mar 22, 2021

monopole reviewed Mar 22, 2021

View reviewed changes

natasha41575 force-pushed the ReplacementFilter branch from 14b64b1 to b920d64 Compare March 23, 2021 20:35

natasha41575 requested a review from KnVerey March 23, 2021 20:38

natasha41575 mentioned this pull request Mar 23, 2021

remove old untested replacement transformer #3747

Merged

monopole reviewed Mar 24, 2021

View reviewed changes

KnVerey reviewed Mar 24, 2021

View reviewed changes

api/filters/replacement/replacement_test.go Outdated Show resolved Hide resolved

KnVerey reviewed Mar 24, 2021

View reviewed changes

api/types/replacement.go Show resolved Hide resolved

natasha41575 force-pushed the ReplacementFilter branch from b920d64 to 9e4aac0 Compare March 29, 2021 18:53

natasha41575 requested review from monopole and KnVerey March 29, 2021 18:55

natasha41575 force-pushed the ReplacementFilter branch 2 times, most recently from 6e9034d to 7560846 Compare March 30, 2021 23:35

monopole approved these changes Mar 31, 2021

View reviewed changes

api/filters/replacement/replacement.go Show resolved Hide resolved

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 31, 2021

natasha41575 force-pushed the ReplacementFilter branch from 7560846 to 0b3f9bd Compare March 31, 2021 20:04

natasha41575 requested a review from monopole March 31, 2021 20:05

KnVerey reviewed Mar 31, 2021

View reviewed changes

add replacement filter to support replacmenttransformer

fa0b237

natasha41575 force-pushed the ReplacementFilter branch from 0b3f9bd to fa0b237 Compare April 1, 2021 00:04

natasha41575 requested a review from KnVerey April 1, 2021 00:05

KnVerey approved these changes Apr 1, 2021

View reviewed changes

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

k8s-ci-robot merged commit ca527a8 into kubernetes-sigs:master Apr 1, 2021

natasha41575 deleted the ReplacementFilter branch April 1, 2021 18:12

This was referenced Apr 2, 2021

add more tests for replacement filter #3771

Merged

Implement use of reject and options fields in replacement filter #3772

Closed

natasha41575 mentioned this pull request Apr 2, 2021

use field options to refine replacements #3773

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add replacement filter to support replacmenttransformer #3735

add replacement filter to support replacmenttransformer #3735

natasha41575 commented Mar 17, 2021 •

edited

Loading

KnVerey left a comment

natasha41575 commented Mar 22, 2021

monopole left a comment

natasha41575 commented Mar 23, 2021

monopole commented Mar 24, 2021

KnVerey left a comment

KnVerey Apr 1, 2021

KnVerey Apr 1, 2021

KnVerey Apr 1, 2021

KnVerey Apr 1, 2021

natasha41575 Apr 1, 2021

k8s-ci-robot commented Apr 1, 2021

	// TODO (#3492): Remove matches listed in the exclude field
	// TODO (#3492): Remove matches listed in the Reject field

	// FieldPath is a structured field path to the desired object
	// FieldOptions refine the interpretation of FieldPaths.

add replacement filter to support replacmenttransformer #3735

add replacement filter to support replacmenttransformer #3735

Conversation

natasha41575 commented Mar 17, 2021 • edited Loading

KnVerey left a comment

Choose a reason for hiding this comment

natasha41575 commented Mar 22, 2021

monopole left a comment

Choose a reason for hiding this comment

natasha41575 commented Mar 23, 2021

monopole commented Mar 24, 2021

KnVerey left a comment

Choose a reason for hiding this comment

KnVerey Apr 1, 2021

Choose a reason for hiding this comment

KnVerey Apr 1, 2021

Choose a reason for hiding this comment

KnVerey Apr 1, 2021

Choose a reason for hiding this comment

KnVerey Apr 1, 2021

Choose a reason for hiding this comment

natasha41575 Apr 1, 2021

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 1, 2021

natasha41575 commented Mar 17, 2021 •

edited

Loading