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
62 changes: 1 addition & 61 deletions cmd/platform/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,10 @@ import (
"github.com/slackapi/slack-cli/cmd/triggers"
internalapp "github.com/slackapi/slack-cli/internal/app"
"github.com/slackapi/slack-cli/internal/cmdutil"
"github.com/slackapi/slack-cli/internal/logger"
"github.com/slackapi/slack-cli/internal/pkg/platform"
"github.com/slackapi/slack-cli/internal/prompts"
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/slackapi/slack-cli/internal/slacktrace"
"github.com/slackapi/slack-cli/internal/style"
"github.com/spf13/cobra"
)
Expand Down Expand Up @@ -144,68 +142,10 @@ func RunRunCommand(clients *shared.ClientFactory, cmd *cobra.Command, args []str
OrgGrantWorkspaceID: runFlags.orgGrantWorkspaceID,
}

log := newRunLogger(clients, cmd)

// Run dev app locally
if _, _, err := runFunc(ctx, clients, log, runArgs); err != nil {
if _, err := runFunc(ctx, clients, runArgs); err != nil {
return err
}

return nil
}

// newRunLogger creates a logger instance to receive event notifications
func newRunLogger(clients *shared.ClientFactory, cmd *cobra.Command) *logger.Logger {
ctx := cmd.Context()
return logger.New(
// OnEvent
func(event *logger.LogEvent) {
switch event.Name {
case "on_update_app_install":
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: This event was never used, disappointingly 😢

cmd.Println(style.Secondary(fmt.Sprintf(
`Updating local app install for "%s"`,
event.DataToString("teamName"),
)))
case "on_cloud_run_connection_connected":
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 here

clients.IO.PrintTrace(ctx, slacktrace.PlatformRunReady)
cmd.Println(style.Secondary("Connected, awaiting events"))
case "on_cloud_run_connection_message":
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 here

message := event.DataToString("cloud_run_connection_message")
clients.IO.PrintDebug(ctx, "received: %s", message)
case "on_cloud_run_connection_command_error":
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 here

message := event.DataToString("cloud_run_connection_command_error")
clients.IO.PrintError(ctx, "Error: %s", message)
case "on_cloud_run_watch_error":
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 to 3 places

message := event.DataToString("cloud_run_watch_error")
clients.IO.PrintError(ctx, "Error: %s", message)
case "on_cloud_run_watch_manifest_change":
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 here

path := event.DataToString("cloud_run_watch_manifest_change")
cmd.Println(style.Secondary(fmt.Sprintf("Manifest change detected: %s, reinstalling app...", path)))
case "on_cloud_run_watch_manifest_change_reinstalled":
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 here

cmd.Println(style.Secondary("App successfully reinstalled"))
case "on_cloud_run_watch_manifest_change_skipped_remote":
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 here

path := event.DataToString("cloud_run_watch_manifest_change_skipped")
cmd.Println(style.Secondary(fmt.Sprintf("Manifest change detected: %s, skipped reinstalling app because manifest.source=remote", path)))
case "on_cloud_run_watch_app_change":
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 here

path := event.DataToString("cloud_run_watch_app_change")
cmd.Println(style.Secondary(fmt.Sprintf("App change detected: %s, restarting server...", path)))
case "on_cleanup_app_install_done":
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 here

cmd.Println(style.Secondary(fmt.Sprintf(
`Cleaned up local app install for "%s".`,
event.DataToString("teamName"),
)))
case "on_cleanup_app_install_failed":
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 here

cmd.Println(style.Secondary(fmt.Sprintf(
`Cleaning up local app install for "%s" failed.`,
event.DataToString("teamName"),
)))
message := event.DataToString("on_cleanup_app_install_error")
clients.IO.PrintWarning(ctx, "Local app cleanup failed: %s", message)
case "on_abort_cleanup_app_install":
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: Never called

cmd.Println(style.Secondary("Aborting, local app might not be cleaned up."))
default:
// Ignore the event
}
},
)
}
13 changes: 6 additions & 7 deletions cmd/platform/run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (

"github.com/slackapi/slack-cli/internal/cmdutil"
"github.com/slackapi/slack-cli/internal/hooks"
"github.com/slackapi/slack-cli/internal/logger"
"github.com/slackapi/slack-cli/internal/pkg/platform"
"github.com/slackapi/slack-cli/internal/prompts"
"github.com/slackapi/slack-cli/internal/shared"
Expand Down Expand Up @@ -48,9 +47,9 @@ type RunPkgMock struct {
mock.Mock
}

func (m *RunPkgMock) Run(ctx context.Context, clients *shared.ClientFactory, log *logger.Logger, runArgs platform.RunArgs) (*logger.LogEvent, types.InstallState, error) {
args := m.Called(ctx, clients, log, runArgs)
return log.SuccessEvent(), types.InstallSuccess, args.Error(0)
func (m *RunPkgMock) Run(ctx context.Context, clients *shared.ClientFactory, runArgs platform.RunArgs) (types.InstallState, error) {
args := m.Called(ctx, clients, runArgs)
return types.InstallSuccess, args.Error(0)
}

func TestRunCommand_Flags(t *testing.T) {
Expand Down Expand Up @@ -222,7 +221,7 @@ func TestRunCommand_Flags(t *testing.T) {

runPkgMock := new(RunPkgMock)
runFunc = runPkgMock.Run
runPkgMock.On("Run", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
runPkgMock.On("Run", mock.Anything, mock.Anything, mock.Anything).Return(nil)

cmd := NewRunCommand(clients)
testutil.MockCmdIO(clients.IO, cmd)
Expand All @@ -234,7 +233,7 @@ func TestRunCommand_Flags(t *testing.T) {
// Check args passed into the run function
if tc.expectedErr == nil {
assert.NoError(t, err)
runPkgMock.AssertCalled(t, "Run", mock.Anything, mock.Anything, mock.Anything,
runPkgMock.AssertCalled(t, "Run", mock.Anything, mock.Anything,
tc.expectedRunArgs,
)
} else {
Expand All @@ -256,7 +255,7 @@ func TestRunCommand_Help(t *testing.T) {

runPkgMock := new(RunPkgMock)
runFunc = runPkgMock.Run
runPkgMock.On("Run", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
runPkgMock.On("Run", mock.Anything, mock.Anything, mock.Anything).Return(nil)

err := cmd.ExecuteContext(ctx)
assert.NoError(t, err)
Expand Down
35 changes: 13 additions & 22 deletions internal/pkg/platform/localserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,12 @@ import (
"github.com/slackapi/slack-cli/internal/goutils"
"github.com/slackapi/slack-cli/internal/hooks"
"github.com/slackapi/slack-cli/internal/iostreams"
"github.com/slackapi/slack-cli/internal/logger"
"github.com/slackapi/slack-cli/internal/pkg/apps"
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/shared/types"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/slackapi/slack-cli/internal/slacktrace"
"github.com/slackapi/slack-cli/internal/style"
)

// for lazy testing
Expand Down Expand Up @@ -78,7 +79,6 @@ type LocalHostedContext struct {
// response management to the script hook
type LocalServer struct {
clients *shared.ClientFactory
log *logger.Logger
token string
localHostedContext LocalHostedContext
cliConfig hooks.SDKCLIConfig
Expand Down Expand Up @@ -143,7 +143,8 @@ func (r *LocalServer) Start(ctx context.Context) error {

// Listen waits for incoming events over a socket connection and invokes the deno-sdk-powered app with each payload. Responds to each event in a way that mimics the behaviour of a hosted app.
func (r *LocalServer) Listen(ctx context.Context, errChan chan<- error, done chan<- bool) {
r.log.Info("on_cloud_run_connection_connected")
r.clients.IO.PrintTrace(ctx, slacktrace.PlatformRunReady)
r.clients.IO.PrintInfo(ctx, false, "%s", style.Secondary("Connected, awaiting events"))

// Listen for socket messages
for {
Expand All @@ -169,8 +170,7 @@ func (r *LocalServer) Listen(ctx context.Context, errChan chan<- error, done cha
}
return
}
r.log.Data["cloud_run_connection_message"] = string(messageBytes)
r.log.Debug("on_cloud_run_connection_message")
r.clients.IO.PrintDebug(ctx, "received: %s", string(messageBytes))

var msg Message
err = json.Unmarshal(messageBytes, &msg)
Expand Down Expand Up @@ -222,13 +222,10 @@ func (r *LocalServer) Listen(ctx context.Context, errChan chan<- error, done cha

if err != nil {
// Log the error but do not return because the user may be able to recover inside their app code
r.log.Data["cloud_run_connection_command_error"] = fmt.Sprintf("%s\n%s", err, out)
r.log.Warn("on_cloud_run_connection_command_error")
r.clients.IO.PrintError(ctx, "Error: %s\n%s", err, out)
break
}

r.log.Info("on_cloud_run_connection_command_output")
Copy link
Member

Choose a reason for hiding this comment

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

🪬 note: I don't believe this log was output either!

🗣️ ramble: Inlining these outputs makes me so much more confident that we're outputting messages we expect more often. I saw in #367 that the run command now shows details on app installation progress. These are such appreciated changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I saw in #367 that the run command now shows details on app installation progress. These are such appreciated changes.

Heh, I just noticed that while testing this PR as well. I had to go back to our latest production release to double-check that I wasn't crazy 🤪


linkResponse = &LinkResponse{
EnvelopeID: msg.EnvelopeID,
Payload: json.RawMessage(out),
Expand Down Expand Up @@ -407,23 +404,19 @@ func (r *LocalServer) WatchManifest(ctx context.Context, auth types.SlackAuth, a
return
case event := <-w.Event:
if isRemoteManifest {
r.log.Data["cloud_run_watch_manifest_change_skipped"] = event.Path
r.log.Info("on_cloud_run_watch_manifest_change_skipped_remote")
r.clients.IO.PrintInfo(ctx, false, "%s", style.Secondary(fmt.Sprintf("Manifest change detected: %s, skipped reinstalling app because manifest.source=remote", event.Path)))
} else {
r.log.Data["cloud_run_watch_manifest_change"] = event.Path
r.log.Info("on_cloud_run_watch_manifest_change")
r.clients.IO.PrintInfo(ctx, false, "%s", style.Secondary(fmt.Sprintf("Manifest change detected: %s, reinstalling app...", event.Path)))

// Reinstall the app when manifest changes
if _, _, _, err := apps.InstallLocalApp(ctx, r.clients, "", auth, app); err != nil {
r.log.Data["cloud_run_watch_error"] = err.Error()
r.log.Warn("on_cloud_run_watch_error")
r.clients.IO.PrintError(ctx, "Error: %s", err)
} else {
r.log.Info("on_cloud_run_watch_manifest_change_reinstalled")
r.clients.IO.PrintInfo(ctx, false, "%s", style.Secondary("App successfully reinstalled"))
}
}
case err := <-w.Error:
r.log.Data["cloud_run_watch_error"] = err.Error()
r.log.Warn("on_cloud_run_watch_error")
r.clients.IO.PrintError(ctx, "Error: %s", err)
case <-w.Closed:
return
}
Expand Down Expand Up @@ -488,8 +481,7 @@ func (r *LocalServer) WatchApp(ctx context.Context) error {
r.clients.IO.PrintDebug(ctx, "App file watcher context canceled, returning.")
return
case event := <-w.Event:
r.log.Data["cloud_run_watch_app_change"] = event.Path
r.log.Info("on_cloud_run_watch_app_change")
r.clients.IO.PrintInfo(ctx, false, "%s", style.Secondary(fmt.Sprintf("App change detected: %s, restarting server...", event.Path)))
Comment on lines -491 to +484
Copy link
Member

Choose a reason for hiding this comment

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

⭐ praise: Inlining variables with outputs makes this more simple to reason about! We've talked about styling within pkg but I remain optimistic for this surfacing within perhaps cmd in the future!

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed on both points! Some of these outputs are complex and passing the variables through another layer has led to a number of issues - some of which we're just noticing as we flatten it!

internal/pkg is serving it's purpose well - as a shameful reminder that it must go away. I agree that moving more into cmd/ seems to be the right choice.


// Stop the previous process before starting a new one
r.stopDelegateProcess(ctx)
Expand All @@ -503,8 +495,7 @@ func (r *LocalServer) WatchApp(ctx context.Context) error {
}
}()
case err := <-w.Error:
r.log.Data["cloud_run_watch_error"] = err.Error()
r.log.Warn("on_cloud_run_watch_error")
r.clients.IO.PrintError(ctx, "Error: %s", err)
case <-w.Closed:
return
}
Expand Down
9 changes: 0 additions & 9 deletions internal/pkg/platform/localserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (
"github.com/gorilla/websocket"
"github.com/slackapi/slack-cli/internal/api"
"github.com/slackapi/slack-cli/internal/hooks"
"github.com/slackapi/slack-cli/internal/logger"
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/slackcontext"
"github.com/slackapi/slack-cli/internal/slackerror"
Expand Down Expand Up @@ -140,12 +139,8 @@ func Test_LocalServer_Start(t *testing.T) {
AppID: "A12345",
TeamID: "justiceleague",
}
log := logger.Logger{
Data: map[string]interface{}{},
}
server := LocalServer{
clients,
&log,
"ABC123",
localContext,
clients.SDKConfig,
Expand Down Expand Up @@ -353,12 +348,8 @@ func Test_LocalServer_Listen(t *testing.T) {
AppID: "A12345",
TeamID: "justiceleague",
}
log := logger.Logger{
Data: map[string]interface{}{},
}
server := LocalServer{
clients,
&log,
"ABC123",
localContext,
clients.SDKConfig,
Expand Down
40 changes: 20 additions & 20 deletions internal/pkg/platform/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,12 @@ import (
"github.com/slackapi/slack-cli/cmd/feedback"
"github.com/slackapi/slack-cli/cmd/triggers"
"github.com/slackapi/slack-cli/internal/config"
"github.com/slackapi/slack-cli/internal/logger"
"github.com/slackapi/slack-cli/internal/pkg/apps"
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/shared/types"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/slackapi/slack-cli/internal/slacktrace"
"github.com/slackapi/slack-cli/internal/style"
)

// RunArgs are the arguments passed into the Run function
Expand All @@ -43,7 +43,7 @@ type RunArgs struct {
}

// Run locally runs your app.
func Run(ctx context.Context, clients *shared.ClientFactory, log *logger.Logger, runArgs RunArgs) (*logger.LogEvent, types.InstallState, error) {
func Run(ctx context.Context, clients *shared.ClientFactory, runArgs RunArgs) (types.InstallState, error) {
span, ctx := opentracing.StartSpanFromContext(ctx, "cmd.create")
defer span.Finish()

Expand All @@ -54,9 +54,9 @@ func Run(ctx context.Context, clients *shared.ClientFactory, log *logger.Logger,
authSession, err := clients.API().ValidateSession(ctx, runArgs.Auth.Token)
if err != nil {
err = slackerror.Wrap(err, "No auth session found")
return nil, "", slackerror.Wrap(err, slackerror.ErrLocalAppRun)
return "", slackerror.Wrap(err, slackerror.ErrLocalAppRun)
}
log.Data["teamName"] = *authSession.TeamName
teamName := *authSession.TeamName

if authSession.UserID != nil {
ctx = config.SetContextUserID(ctx, *authSession.UserID)
Expand All @@ -73,17 +73,17 @@ func Run(ctx context.Context, clients *shared.ClientFactory, log *logger.Logger,
// For CLI Run command execute successfully
if !clients.SDKConfig.Hooks.Start.IsAvailable() {
var err = slackerror.New(slackerror.ErrSDKHookNotFound).WithMessage("The `start` script was not found")
return nil, "", err
return "", err
}

// Update local install
installedApp, localInstallResult, installState, err := apps.InstallLocalApp(ctx, clients, runArgs.OrgGrantWorkspaceID, runArgs.Auth, runArgs.App)
if err != nil {
return nil, "", slackerror.Wrap(err, slackerror.ErrLocalAppRun)
return "", slackerror.Wrap(err, slackerror.ErrLocalAppRun)
}

if installState == types.InstallRequestPending || installState == types.InstallRequestCancelled || installState == types.InstallRequestNotSent {
return log.SuccessEvent(), types.InstallSuccess, nil
return types.InstallSuccess, nil
}

if runArgs.ShowTriggers {
Expand All @@ -93,17 +93,17 @@ func Run(ctx context.Context, clients *shared.ClientFactory, log *logger.Logger,
if strings.Contains(err.Error(), "workflow_not_found") {
listErr := triggers.ListWorkflows(ctx, clients, runArgs.App, runArgs.Auth)
if listErr != nil {
return nil, "", slackerror.Wrap(listErr, "Error listing workflows").WithRootCause(err)
return "", slackerror.Wrap(listErr, "Error listing workflows").WithRootCause(err)
}
}
return nil, "", err
return "", err
}
}

// Gather environment variables from an environment file
variables, err := clients.Config.GetDotEnvFileVariables()
if err != nil {
return nil, "", slackerror.Wrap(err, slackerror.ErrLocalAppRun).
return "", slackerror.Wrap(err, slackerror.ErrLocalAppRun).
Copy link
Member

Choose a reason for hiding this comment

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

🎆 praise: Wrapping errors is a most exciting pattern we should continue to support!

WithMessage("Failed to read the local .env file")
}

Expand All @@ -123,7 +123,6 @@ func Run(ctx context.Context, clients *shared.ClientFactory, log *logger.Logger,

var server = LocalServer{
clients: clients,
log: log,
token: localInstallResult.APIAccessTokens.AppLevel,
localHostedContext: localHostedContext,
cliConfig: cliConfig,
Expand All @@ -141,7 +140,7 @@ func Run(ctx context.Context, clients *shared.ClientFactory, log *logger.Logger,
<-ctx.Done()
clients.IO.PrintDebug(ctx, "Interrupt signal received in Run command, cleaning up and shutting down")
if cleanup {
deleteAppOnTerminate(ctx, clients, runArgs.Auth, installedApp, log)
deleteAppOnTerminate(ctx, clients, runArgs.Auth, installedApp, teamName)
}
feedback.ShowFeedbackMessageOnTerminate(ctx, clients)
// Notify Slack backend we are closing connection; this should trigger an echoing close message from Slack
Expand Down Expand Up @@ -196,21 +195,22 @@ func Run(ctx context.Context, clients *shared.ClientFactory, log *logger.Logger,
if err := <-errChan; err != nil {
switch slackerror.ToSlackError(err).Code {
case slackerror.ErrLocalAppRunCleanExit:
return log.SuccessEvent(), types.InstallSuccess, nil
return types.InstallSuccess, nil
case slackerror.ErrSDKHookInvocationFailed:
return nil, "", err
return "", err
}
return nil, "", slackerror.Wrap(err, slackerror.ErrLocalAppRun)
return "", slackerror.Wrap(err, slackerror.ErrLocalAppRun)
}
return log.SuccessEvent(), types.InstallSuccess, nil
return types.InstallSuccess, nil
}

func deleteAppOnTerminate(ctx context.Context, clients *shared.ClientFactory, auth types.SlackAuth, app types.App, log *logger.Logger) {
func deleteAppOnTerminate(ctx context.Context, clients *shared.ClientFactory, auth types.SlackAuth, app types.App, teamName string) {
clients.IO.PrintDebug(ctx, "Removing the local version of this app from the workspace")
_, _, err := apps.Delete(ctx, clients, app.TeamDomain, app, auth)
if err != nil {
log.Data["on_cleanup_app_install_error"] = err.Error()
log.Info("on_cleanup_app_install_failed")
clients.IO.PrintInfo(ctx, false, "%s", style.Secondary(fmt.Sprintf(`Cleaning up local app install for "%s" failed.`, teamName)))
clients.IO.PrintWarning(ctx, "Local app cleanup failed: %s", err)
} else {
clients.IO.PrintInfo(ctx, false, "%s", style.Secondary(fmt.Sprintf(`Cleaned up local app install for "%s".`, teamName)))
}
log.Info("on_cleanup_app_install_done")
}
Comment on lines +207 to 216
Copy link
Member

Choose a reason for hiding this comment

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

💡 thought(non-blocking): Using a familiar iostreams package for these outputs might make this inlined logic more simple to test also! Thanks so much for finding these changes 🎁

Loading