-
Notifications
You must be signed in to change notification settings - Fork 24
feat: error if the runtime is not found with a missing hook #167
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #167 +/- ##
=======================================
Coverage 63.07% 63.08%
=======================================
Files 212 212
Lines 21667 21672 +5
=======================================
+ Hits 13666 13671 +5
+ Misses 6951 6950 -1
- Partials 1050 1051 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zimeg
left a comment
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.
📝 Leaving a few notes for the kind reviewers!
| if slackerror.Is(err, slackerror.ErrSDKHookNotFound) && clients.SDKConfig.Runtime == "" { | ||
| err = slackerror.New(slackerror.ErrRuntimeNotFound). | ||
| WithRootCause(slackerror.ToSlackError(err).WithRemediation("")) | ||
| } |
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.
🔏 note: The runtime is unknown to actual hook scripts, so the runtime_not_found error can't be determined during hook execution itself...
🤔 question: Does this error handling logic make sense to include at the root command cleanup, or should hook executions be checked for this?
| clients.EventTracker.SetErrorMessage(err.Error()) | ||
| if slackErr, ok := err.(*slackerror.Error); ok { | ||
| clients.EventTracker.SetErrorCode(slackErr.Code) |
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.
🔍 note: Moved this since we now might update the err value!
|
🗣️ note: This is followup in preparation for changes from #165! |
mwbrooks
left a comment
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.
✅ Thanks for updating this! It's a very nice improvement to debug why a hook has failed.
🧪 Manual testing worked well - I tested using Deno. 🦕
|
|
||
| func triggerRequestViaHook(ctx context.Context, clients *shared.ClientFactory, path string, isDev bool) (api.TriggerRequest, error) { | ||
| if !clients.SDKConfig.Hooks.GetTrigger.IsAvailable() { | ||
| return api.TriggerRequest{}, slackerror.New(slackerror.ErrSDKHookGetTriggerNotFound) |
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.
praise: I like that you've chosen to remove this very-specific hook error for the general ErrSDKHookNotFound and Remediation combination.
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.
thought: Once errors across packages are wrapped I'm optimistic these same details will be clear in output messages 🙏
Summary
This PR errors with a
runtime_not_founderror if the hookruntimedoesn't exist when asdk_hook_not_founderror is returned.With this change, we make use of hook resolution and assume that if the runtime isn't set, that wasn't returned in the response from
get-hooks.In sample applications and with
initwe suggest gathering hook values from theget-hooksscript and don't override theruntimevalue. IMO this is good for the happy path cases! Additional hook customizations might run into unexpected errors with a customizedruntimebut debugging information remains available in verbose outputs 👾Preview
🔍 Errors when a hook is not found and the runtime doesn't exist follow:
🔍 Errors when a hook is found but errors, without a runtime, continue to error:
Reviewers
Uninstall various runtimes and attempt to interact with a matching app:
Notes
get-hooksscript errors is needed so that other hooks can still be parsed, if defined.Requirements