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

kpt fn render ensures validators don't mutate resources #1896

Merged
merged 3 commits into from May 5, 2021

Conversation

droot
Copy link
Contributor

@droot droot commented May 4, 2021

With this PR, kpt fn render ensures that validators in a pipeline don't mutate the resources.

This is achieved by splitting the execution of mutators from validators. Now we run validators on a copy of mutated resources.

Note: It will be nice if kpt fn render can flag a validator function that mutates the resources. I think that is doable, but need be investigated how best to do that.

fixes #1860

@droot droot added this to In Review in kpt kanban board via automation May 4, 2021
@droot droot added this to the v1.0 m2 milestone May 4, 2021
@droot droot marked this pull request as draft May 4, 2021 22:07
@droot
Copy link
Contributor Author

droot commented May 4, 2021

Ignore this PR for now, @Shell32-Natsu identified an issue with this, so I am going to fix that.

@droot droot force-pushed the fn-render-split-validators branch from 87e281c to cbe0210 Compare May 5, 2021 06:15
@droot droot marked this pull request as ready for review May 5, 2021 06:20

// runMutators runs a set of mutators functions on given input resources.
func (pn *pkgNode) runMutators(ctx context.Context, hctx *hydrationContext, input []*yaml.RNode) ([]*yaml.RNode, error) {
const op errors.Op = "pipeline.runMutators"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this op, since we don't want to expose internal implementation details to users, I suspect does it really have a use case? Is it only used in debug? Is there a debug mode for kpt? And why don't we directly add a real stack trace here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer useful, we took out the stacktrace printing. Taking it out.

return nil, errors.E(op, pn.pkg.UniquePath, err)
}
// print a new line after a pipeline running
pr.Printf("\n")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the original code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, I remember that I added this. :0

@droot droot requested a review from Shell32-Natsu May 5, 2021 18:08
@droot droot merged commit 20bb4f8 into kptdev:next May 5, 2021
kpt kanban board automation moved this from In Review to Done May 5, 2021
frankfarzan pushed a commit to frankfarzan/kpt that referenced this pull request Jun 3, 2021
* kpt fn render ensures validators don't mutate resources
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants