-
Notifications
You must be signed in to change notification settings - Fork 67
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
Improve WASM runtime error messages by adding a function to retrieve errors from resourceList #978
Improve WASM runtime error messages by adding a function to retrieve errors from resourceList #978
Conversation
// Currently error is surfaced as a string. | ||
jsonFunc := js.FuncOf(func(this js.Value, args []js.Value) interface{} { | ||
func resourceListProcessorWrapper(resourceList *[]byte) js.Func { | ||
jsonFunc := js.FuncOf(func(this js.Value, args []js.Value) any { | ||
if len(args) != 1 { | ||
return "Invalid number of arguments passed" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this entire function could simply either return the resourceList or empty string, always deferring to the other function to get error messages, rather than mixing those approaches. Would require an update to kptdev/kpt#3830 if we take that route.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR! Could you add some unit/e2e tests to verify and document the behavior of this change? |
Thank you so much for making the change. The PR makes a lot of sense to me. I just have a comment about the potential risk conditions to pull out the Can you also share the your testing steps (test data, command to reproduce) in the PR description? |
$ kpt fn eval --image gcr.io/kpt-fn/apply-setters:unstable -- image=ubuntu replicas=3 | ||
$ kpt fn eval --image gcr.io/kpt-fn/apply-setters:unstable -- tag=1.16.2 nginx-replicas=3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qq: do we need to change this example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I had no intention of making this change btw. It probably was made when running make
)
// Currently error is surfaced as a string. | ||
jsonFunc := js.FuncOf(func(this js.Value, args []js.Value) interface{} { | ||
func resourceListProcessorWrapper(resourceList *[]byte) js.Func { | ||
jsonFunc := js.FuncOf(func(this js.Value, args []js.Value) any { | ||
if len(args) != 1 { | ||
return "Invalid number of arguments passed" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return "" | ||
} | ||
errorMessages := "" | ||
for _, r := range(rl.Results) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When running in a pipeline, the function only appends Results
to ResourceList.Results
but not override the existing ones. So this resourceListProcessorErrors
may get irrelevant error Results
(from previous functions' results).
I'd suggest at least adding a comment to notify future contributors about this since it could cause some unexpected failures in certain risk conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added!
I've added a copy & paste from the linked issue at the top of this PR, as those resources can be used to reproduce. |
I can do this, although it will take some time (don't have a golang background) |
@wleese If there is a good way to add unit tests, that would be nice, but I don't think we have an existing e2e framework for wasm functions and I don't want to block this PR on that. Adding e2e tests for wasm functions is a separate issue (kptdev/kpt#3587) that could take a while to do. So if you find yourself stuck on a good way to add tests, don't worry about spending too much time on it and IMO we can still accept this PR without them. |
Good catch and suggestion, @sdowell and @natasha41575 Yeah, I think adding the test steps in the PR description is good enough for this PR. We'd like to help and make the contribution experience easier for active users and contributors. |
Thanks! |
We'd like WASM compiled functions to provide the same information as those run in docker and aims to solve kptdev/kpt#3822
This PR also resolves a panic that can occur when an
error
is returned in the constructed javascript function.In order to actually surface the error messages, the kpt wasm function runner needs to be modified.
This can be tested with:
Kptfile
resources.yaml
starlarkconfig.yaml
And running:
kpt fn render . --allow-alpha-wasm
and
KPT_FN_WASM_RUNTIME=nodejs kpt fn render . --allow-alpha-wasm