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

proper error message for fn eval/render when there are non-KRM resources #2198

Merged
merged 9 commits into from
Jun 14, 2021

Conversation

natasha41575
Copy link
Contributor

@natasha41575 natasha41575 commented Jun 8, 2021

partially addresses #2039, improves error message for fn eval and fn render when there are non-KRM resources

Before:

[FAIL] "gcr.io/kpt-fn/set-namespace:v0.1.3"
  Results:
    [ERROR] failed to run function: error unmarshaling JSON: while decoding JSON: Object 'Kind' is missing in '{"Non-krm-resource":true,"metadata":{"annotations":{"config.k8s.io/id":"1","config.kubernetes.io/index":"0","config.kubernetes.io/path":"nonkrm.yaml"}}}'
  Stderr:
    "[error] /// : failed to run function: error unmarshaling JSON: while decoding JSON: Object 'Kind' is missing in '{\"Non-krm-resource\":true,\"metadata\":{\"annotations\":{\"config.k8s.io/id\":\"1\",\"config.kubernetes.io/index\":\"0\",\"config.kubernetes.io/path\":\"nonkrm.yaml\"}}}'"
  Exit code: 1

After:

Error: input resource list must contain only KRM resources:
non-krm.yaml: resource must have `apiVersion`

Will do the same for fn source and fn sink in a separate PR.

pkg/api/kptfile/v1alpha2/validation.go Outdated Show resolved Hide resolved
pkg/api/kptfile/v1alpha2/validation.go Outdated Show resolved Hide resolved
thirdparty/kyaml/runfn/runfn.go Outdated Show resolved Hide resolved
@droot
Copy link
Contributor

droot commented Jun 9, 2021

I am actually thinking of another approach. It's important to enforce this invariant on input/output resources of each function execution(doing it in fnruntime can cover both eval and render). Use the path/index to derive a user-friendly location (2nd resource in <some.yaml>) to report an error (avoid dumping the yaml content). One caveat here: for generated resources, the path will be auto generated one, but that's okay I think.

Doing above is not sufficient to ensure the package is valid because if a package doesn't have any pipeline, function runner will not be invoked, we can have an explicit call for such cases (I think , in function named runPipeline, we return if pipeline.IsEmpty(), we can plug call for KRM check in that case). [This is not an issue for fn eval]

So, tests should include case where non-KRM resources with no pipeline as well.

@natasha41575
Copy link
Contributor Author

I am actually thinking of another approach. It's important to enforce this invariant on input/output resources of each function execution(doing it in fnruntime can cover both eval and render). Use the path/index to derive a user-friendly location (2nd resource in <some.yaml>) to report an error (avoid dumping the yaml content). One caveat here: for generated resources, the path will be auto generated one, but that's okay I think.

Will do

@droot droot added this to the v1.0 m3 milestone Jun 9, 2021
@droot droot added this to In Review in kpt kanban board via automation Jun 9, 2021
@natasha41575
Copy link
Contributor Author

@droot I moved the check to fnruntime and added a test case for no pipeline, thanks!

@mikebz mikebz removed this from the v1.0 m3 milestone Jun 10, 2021
@natasha41575
Copy link
Contributor Author

@droot please take a look, thanks

Copy link
Contributor

@frankfarzan frankfarzan left a comment

Choose a reason for hiding this comment

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

please wait for @droot

Copy link
Contributor

@droot droot left a comment

Choose a reason for hiding this comment

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

New approach is neat! Thank you.

@natasha41575 natasha41575 merged commit 0e00495 into kptdev:main Jun 14, 2021
kpt kanban board automation moved this from In Review to Done Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

fn render/eval/source/sink should generate proper error message when there are non-KRM resources
4 participants