-
Notifications
You must be signed in to change notification settings - Fork 29
refactor: remove internal/logger from platform run #378
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
Changes from all commits
83f47e1
ff6c2c4
ea5b166
a2b971d
55066b9
32a741d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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" | ||
| ) | ||
|
|
@@ -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": | ||
| cmd.Println(style.Secondary(fmt.Sprintf( | ||
| `Updating local app install for "%s"`, | ||
| event.DataToString("teamName"), | ||
| ))) | ||
| case "on_cloud_run_connection_connected": | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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": | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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": | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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": | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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": | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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": | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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": | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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": | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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": | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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": | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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": | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
| } | ||
| }, | ||
| ) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -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 | ||
|
|
@@ -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 { | ||
|
|
@@ -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) | ||
|
|
@@ -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") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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), | ||
|
|
@@ -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 | ||
| } | ||
|
|
@@ -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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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!
|
||
|
|
||
| // Stop the previous process before starting a new one | ||
| r.stopDelegateProcess(ctx) | ||
|
|
@@ -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 | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -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() | ||
|
|
||
|
|
@@ -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) | ||
|
|
@@ -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 { | ||
|
|
@@ -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). | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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") | ||
| } | ||
|
|
||
|
|
@@ -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, | ||
|
|
@@ -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 | ||
|
|
@@ -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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 thought(non-blocking): Using a familiar |
||
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: This event was never used, disappointingly 😢