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.

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

Add support for tracking mutations applied by kustomize filters #4372

Closed
sdowell opened this issue Jan 7, 2022 · 5 comments
Closed

Add support for tracking mutations applied by kustomize filters #4372

sdowell opened this issue Jan 7, 2022 · 5 comments
Labels
area/api issues for api module kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@sdowell
Copy link
Contributor

sdowell commented Jan 7, 2022

Is your feature request related to a problem? Please describe.

As a consumer of the filters library, I would like the functionality to track mutations which have been applied to the input. This would help with use cases such as emitting the resources/fields which have been modified after applying a filter. Ideally the functionality would be provided in a consistent manner across all of the filters.

Describe the solution you'd like

This functionality has been added to the annotations and labels filter by the respective PRs:
annotations: #4307
labels: #4369

Given that this implementation is well received and generically useful, I suggest that this functionality be provided consistently across all of the filters. It would be preferable to have consistent functionality across all of the filters rather than only offer this functionality on a subset of the filters.

Describe alternatives you've considered

It is my understanding that there is not a more general way to provide this functionality (given the current filter design/implementation), and the functionality needs to be provided by each filter individually.

@sdowell sdowell added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 7, 2022
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jan 7, 2022
@natasha41575
Copy link
Contributor

I find adding a generic, configurable callback function to each filter to be very useful, particularly for the use case described here. It allows information, such as which field paths were affected. to become available outside the filter, which has vastly improved our function authoring experience. It is now possible to use the filter to tell you which files, and which fields in the resources of those files, have been affected, which is a much better user experience.

In general it makes sense to me that if we add a generic, configurable callback function for one filter, then it would be good to provide them in all filters. Those using kustomize filters as a library will be given some flexibility in how they choose to use the filters and will be able to access to some important information that is otherwise not available outside the filter. It will also be good to maintain some consistency - if we agree that this functionality is useful, then it seems odd to me to support it in just some filters and not others.

An alternative that comes to mind is to have each filter return a list of fieldpaths that were affected, but this may be too specific to one use case and may not support other types of structured results.

/area api
/kind feature
/triage accepted

@k8s-ci-robot k8s-ci-robot added area/api issues for api module triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 7, 2022
@natasha41575
Copy link
Contributor

natasha41575 commented Jan 12, 2022

Discussed with @KnVerey and she suggested that before getting started on these PRs, we first have a thorough idea of what the changes are going to be.

Would you mind listing out all the filters you are going to change, as well as the implementation details of the changes you are going to make for each? One thing that came up in discussion was that the name setEntryCallback probably doesn't make sense for the other filters, so it would be good to see that the generic solution that we have really will be generic and able to be applied to all filters.

Another suggestion made was to add the method to the Filter interface, rather than adding the function as a field to each filter individually. This approach ensures consistency across each of the filters and I'm curious if it would equally serve your purpose or if there are other issues you would encounter with it. Though this would be less of a "configurable callback function" and more of something that specifically targets your use case. For these reasons I slightly favor keeping it as a configurable field but would be open to more discussion on this.

@KnVerey
Copy link
Contributor

KnVerey commented Jan 14, 2022

Clarification on my suggestion about interfaces: I don't think we should change the kio.Filter interface, which is very, very simple and probably used outside Kustomize. Rather, I was thinking of a new interface with a superset of kio.Filter's functions, e.g. WithMutationTracker(func(key, value, tag string, node *yaml.RNode)) in addition to Filter([]*yaml.RNode) ([]*yaml.RNode, error). We'd then make all our filters internally satisfy both. A consumer iterating over a list of kio.Filter produced by us and others could then use type assertions to conditionally add the mutation tracker to each one without resolving it to the underlying type. Does that make sense?

@natasha41575
Copy link
Contributor

@KnVerey Thank you for clarifying! After looking through the filters @sdowell suggested a similar thing that could apply to a subset of filters because we noticed that the filters are difficult to generalize and tracking the changes field paths doesn’t make sense for all of them (such as patchesJson6902). I suggested playing around with fsslice a bit and from there Sam came up with a different and perhaps less invasive alternative that would live largely in filtersutil. @sdowell could you share the details here?

I am also open to introducing a new interface but I’m curious to see a more fleshed out version of the idea Sam shared with me.

@sdowell
Copy link
Contributor Author

sdowell commented Jan 14, 2022

@KnVerey Thank you for the suggestion, I like the idea of creating a new interface with a superset of the functionality provided by kio.Filter. As @natasha41575 explained (thank you @natasha41575), it seems that it will not be feasible to extend this functionality to all of the filters (e.g. patchjson6902) - so this would be applied to a subset of the filters.

The filters I have in mind currently are:
annotations, imagetag, labels, namespace, prefix, replicacount, and suffix

There is a common pattern of these filters in that they reuse the abstractions provided by filtersutil. The only exception is imagetag, which I think can be easily refactored to use SetScalar.

My proposal is to provide the callback functionality at the layer of filtersutil and reuse it for the above functions. I put together a quick POC that demonstrates my thinking in this commit:
649826d

To summarize I think we can combine these ideas by creating a new interface along the lines of @KnVerey's suggestion, and under the hood reuse the common implementation illustrated by my commit above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api issues for api module kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

4 participants