Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -376,20 +376,23 @@ func ExecuteContext(ctx context.Context, rootCmd *cobra.Command, clients *shared

// The cleanup() method in the root command will invoke via `defer` from within Execute.
if err := rootCmd.ExecuteContext(ctx); err != nil {
errMsg := err.Error()
clients.EventTracker.SetErrorMessage(errMsg)
if slackErr, ok := err.(*slackerror.Error); ok {
clients.EventTracker.SetErrorCode(slackErr.Code)
}
if slackerror.Is(err, slackerror.ErrProcessInterrupted) {
clients.IO.SetExitCode(iostreams.ExitCancel)
clients.IO.PrintDebug(ctx, errMsg)
clients.IO.PrintDebug(ctx, err.Error())
} else {
if slackerror.Is(err, slackerror.ErrSDKHookNotFound) && clients.SDKConfig.Runtime == "" {
err = slackerror.New(slackerror.ErrRuntimeNotFound).
WithRootCause(slackerror.ToSlackError(err).WithRemediation(""))
}
Comment on lines +383 to +386
Copy link
Member Author

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?

switch clients.IO.GetExitCode() {
case iostreams.ExitOK:
clients.IO.SetExitCode(iostreams.ExitError)
}
clients.IO.PrintError(ctx, errMsg)
clients.IO.PrintError(ctx, err.Error())
}
clients.EventTracker.SetErrorMessage(err.Error())
if slackErr, ok := err.(*slackerror.Error); ok {
clients.EventTracker.SetErrorCode(slackErr.Code)
Comment on lines +393 to +395
Copy link
Member Author

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!

}
defer clients.Os.Exit(int(clients.IO.GetExitCode()))
completedChan <- true
Expand Down
43 changes: 36 additions & 7 deletions cmd/root_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/slackapi/slack-cli/internal/iostreams"
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/slackcontext"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/slackapi/slack-cli/test/testutil"
"github.com/spf13/cobra"
"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -81,21 +82,43 @@ func TestRootCommand(t *testing.T) {

func TestExecuteContext(t *testing.T) {
tests := map[string]struct {
expectedErr error
expectedExitCode iostreams.ExitCode
expectedOutputs []string
mockErr error
mockRuntime string
expectedExitCode iostreams.ExitCode
expectedOutputs []string
unexpectedOutputs []string
}{
"Command successfully executes": {
expectedErr: nil,
mockErr: nil,
expectedExitCode: iostreams.ExitOK,
},
"Command fails execution and returns an error": {
expectedErr: fmt.Errorf("command failed"),
mockErr: fmt.Errorf("command failed"),
expectedExitCode: iostreams.ExitError,
expectedOutputs: []string{
"command failed",
},
},
"Command fails execution with a missing hook and missing runtime": {
mockErr: slackerror.New(slackerror.ErrSDKHookNotFound),
expectedExitCode: iostreams.ExitError,
expectedOutputs: []string{
slackerror.New(slackerror.ErrRuntimeNotFound).
WithRootCause(slackerror.New(slackerror.ErrSDKHookNotFound).WithRemediation("")).
Error(),
},
},
"Command fails execution with a missing hook and existing runtime": {
mockErr: slackerror.New(slackerror.ErrSDKHookNotFound),
mockRuntime: "sh",
expectedExitCode: iostreams.ExitError,
expectedOutputs: []string{
slackerror.New(slackerror.ErrSDKHookNotFound).Error(),
},
unexpectedOutputs: []string{
slackerror.ErrRuntimeNotFound,
},
},
}
for name, tt := range tests {
t.Run(name, func(t *testing.T) {
Expand All @@ -105,15 +128,18 @@ func TestExecuteContext(t *testing.T) {
clientsMock := shared.NewClientsMock()
clientsMock.AddDefaultMocks()
clientsMock.EventTracker.On("FlushToLogstash", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
clients := shared.NewClientFactory(clientsMock.MockClientFactory())
clients := shared.NewClientFactory(clientsMock.MockClientFactory(), func(clients *shared.ClientFactory) {
clients.SDKConfig.Runtime = tt.mockRuntime
})

// Mock command
cmd := &cobra.Command{
Use: "mock [flags]",
RunE: func(cmd *cobra.Command, args []string) error {
return tt.expectedErr
return tt.mockErr
},
}
testutil.MockCmdIO(clientsMock.IO, cmd)

// Execute the command
ExecuteContext(ctx, cmd, clients)
Expand All @@ -126,6 +152,9 @@ func TestExecuteContext(t *testing.T) {
for _, expectedOutput := range tt.expectedOutputs {
require.Contains(t, output, expectedOutput)
}
for _, unexpectedOutputs := range tt.unexpectedOutputs {
require.NotContains(t, output, unexpectedOutputs)
}
})
}
}
Expand Down
5 changes: 4 additions & 1 deletion cmd/triggers/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"context"
"encoding/json"
"fmt"
"path/filepath"
"strings"

"github.com/opentracing/opentracing-go"
Expand Down Expand Up @@ -306,7 +307,9 @@ func triggerRequestFromFlags(flags createCmdFlags, isDev bool) api.TriggerReques

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)
Copy link
Member

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.

Copy link
Member Author

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 🙏

return api.TriggerRequest{}, slackerror.New(slackerror.ErrSDKHookNotFound).
WithMessage("The `get-trigger` hook script in `%s` was not found", filepath.Join(".slack", "hooks.json")).
WithRemediation("Try defining your trigger by specifying a json file instead.")
}

hookExecOpts := hooks.HookExecOpts{
Expand Down
3 changes: 2 additions & 1 deletion cmd/triggers/create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/shared/types"
"github.com/slackapi/slack-cli/internal/slackcontext"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/slackapi/slack-cli/test/testutil"
"github.com/spf13/afero"
"github.com/spf13/cobra"
Expand Down Expand Up @@ -293,7 +294,7 @@ func TestTriggersCreateCommand(t *testing.T) {
},
"--trigger-def, not json, `get-trigger` hook missing": {
CmdArgs: []string{"--trigger-def", "triggers/shortcut.ts"},
ExpectedErrorStrings: []string{"sdk_hook_get_trigger_not_found"},
ExpectedErrorStrings: []string{slackerror.ErrSDKHookNotFound},
Setup: func(t *testing.T, ctx context.Context, clientsMock *shared.ClientsMock, clients *shared.ClientFactory) {
appSelectTeardown = setupMockCreateAppSelection(installedProdApp)
// TODO: testing chicken and egg: we need the default mocks in place before we can use any of the `clients` methods
Expand Down
16 changes: 8 additions & 8 deletions internal/slackerror/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,11 +210,11 @@ const (
ErrRequestIDOrAppIDIsRequired = "request_id_or_app_id_is_required"
ErrRatelimited = "ratelimited"
ErrRestrictedPlanLevel = "restricted_plan_level"
ErrRuntimeNotFound = "runtime_not_found"
ErrRuntimeNotSupported = "runtime_not_supported"
ErrSDKConfigLoad = "sdk_config_load_error"
ErrSDKHookInvocationFailed = "sdk_hook_invocation_failed"
ErrSDKHookNotFound = "sdk_hook_not_found"
ErrSDKHookGetTriggerNotFound = "sdk_hook_get_trigger_not_found"
ErrSampleCreate = "sample_create_error"
ErrServiceLimitsExceeded = "service_limits_exceeded"
ErrSharedChannelDenied = "shared_channel_denied"
Expand Down Expand Up @@ -1291,9 +1291,15 @@ Otherwise start your app for local development with: %s`,
Message: "Your Slack plan does not have access to the requested feature",
},

ErrRuntimeNotFound: {
Code: ErrRuntimeNotFound,
Message: "The hook runtime executable was not found",
Remediation: "Make sure the required runtime has been installed to run hook scripts.",
},

ErrRuntimeNotSupported: {
Code: ErrRuntimeNotSupported,
Message: "The SDK language's executable (deno, node, python, etc) was not found to be installed on the system",
Message: "The SDK runtime is not supported by the CLI",
},

ErrSampleCreate: {
Expand Down Expand Up @@ -1340,12 +1346,6 @@ Otherwise start your app for local development with: %s`,
}, "\n"),
},

ErrSDKHookGetTriggerNotFound: {
Code: ErrSDKHookGetTriggerNotFound,
Message: fmt.Sprintf("The `get-trigger` hook script in `%s` was not found", filepath.Join(".slack", "hooks.json")),
Remediation: `Try defining your trigger by specifying a json file instead.`,
},

ErrSlackAuth: {
Code: ErrSlackAuth,
Message: "You are not logged into a team or have not installed an app",
Expand Down
Loading