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

Handle "Result" errors as non-fatal errors in kyaml FilterFuncs #4241

Merged
merged 2 commits into from Nov 22, 2021

Conversation

Goodwine
Copy link
Contributor

Context & Description

kyaml currently has no way of updating ResourceList.Result unless a custom processor is created by the end user.
This PR updates ResourceList.Filter used by all current processors so whenever the kio.Filter returns an error type *Result it will be assigned to ResourceList.Result instead of erroring out as is the current behavior.

This behavior change in ResourceList.Filter is a breaking change in the API, I believe this should be OK because IIUC this use case is not adopted widely.

Alternative Considered

Instead of special error handling, we can introduce a new Resulter{ Result(...) (*Result, error) } interface that can be implemented by the KRM's Filter(), but when trying this out it feels very non-idiomatic because in most cases where we want a Resulter we would also have a no-op filter like func (myExample) Filter(in) { return in, nil }.

This is also an extra things to be kept in mind from KRM function authors because the interface is not explicitly called out and the behaviors can be unexpected and non idiomatic.

However this does avoid the breaking change.

Workaround
Introduce a new value in Result.Severity called Fatal, the ResourceList.Filter function could verify this value when applicable and throw an actual error instead of keep going.
(Or.. we can make erroring out the default behavior but adding a new Result.NoError flag to trigger this new behavior.

Additional Notes

  • This PR is built on top of PR Fix kyaml readwriter inconsistencies when wrapping resources #4235, the first 2 commits are not part of this change.
  • I also have a function/example that goes with this change but is blocked by this PR being merged because of go-modules
  • This change has been discussed briefly with Katrina Verey and Jeremy Rickard (in case this needs to be discussed in a Kustomize meeting)

@k8s-ci-robot
Copy link
Contributor

@Goodwine: This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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 k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 14, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @Goodwine. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 14, 2021
@Goodwine
Copy link
Contributor Author

/assign @KnVerey

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2021
@Goodwine
Copy link
Contributor Author

Goodwine commented Nov 9, 2021

When looking at the behavior changes needed here I realized that not all processors behaved the same with Filter when Results was returned as an error value, e.g. kio.Pipeline so I went ahead and updated all invocations to handle the results gracefully so it's not only the ResourceListProcessor but also the others, and this required untangling dependencies, especifically moving Results from the framework package into kio

This is quite a change so I'll wait to discuss this further this week

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2021
…re in ResourceList

- Result can only count as error when passed as pointer, this makes easy use of "errors.As"
- Existing Filter() implementations that return Result from the `framework` package won't return an error anymore but modify the ResourceList
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 11, 2021
@Goodwine
Copy link
Contributor Author

After discussing with Katrina over slack we reached out conclusion that the original PR was OK, we don't need to overreach with Results since it's ResourceList-specific

The only 2 key things worth mentioning are

  • kio.Pipeline doesn't collect the results and if this is a wanted behavior a framework.Pipeline could be added in the future but probably not needed in this PR.
  • the runtime package that has starlark code generation for a ResourceList doesn't handle Results either and that is out of scope for this PR

Copy link
Contributor

@KnVerey KnVerey left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold for @natasha41575 to approve

kyaml/fn/framework/framework.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 12, 2021
Co-authored-by: Katrina Verey <kn.verey@gmail.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2021
@KnVerey
Copy link
Contributor

KnVerey commented Nov 12, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2021
@KnVerey
Copy link
Contributor

KnVerey commented Nov 12, 2021

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Nov 12, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 12, 2021
@KnVerey KnVerey added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 12, 2021
@natasha41575
Copy link
Contributor

So sorry for the delay!

/approve

@natasha41575 natasha41575 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 22, 2021
@natasha41575 natasha41575 merged commit 0142076 into kubernetes-sigs:master Nov 22, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Goodwine, 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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2021
@Goodwine Goodwine deleted the kyaml-result-provider branch November 22, 2021 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants