refactor: remove internal/logger from platform run by mwbrooks · Pull Request #378 · slackapi/slack-cli

mwbrooks · 2026-03-10T00:18:40Z

Changelog

N/A

Summary

This pull request is Part 4 of removing internal/logger package and is focused on the the run command.

Test Steps

# Create Deno SDK project
$ lack create my-app -t https://github.com/slack-samples/deno-starter-template
$ cd my-app/

# Run dev server
$ lack run
# → Confirm output formatting looks correct
# CTRL+C

# Restart dev server
$ lack run
# → Confirm output formatting looks correct

# Change file: Manifest
$ vim manifest.ts
# → Change the description and save
# → Confirm "Manifest change detected:"
# CTRL+C

# Clean up
$ lack delete -f
$ cd ..
$ rm -rf my-app/

Requirements

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

…, delete, and uninstall

# Conflicts: # cmd/project/create.go # cmd/project/create_test.go # internal/pkg/apps/install.go # internal/pkg/platform/localserver.go

codecov · 2026-03-10T00:45:37Z

Codecov Report

❌ Patch coverage is 12.90323% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.43%. Comparing base (2ae86a3) to head (32a741d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/pkg/platform/run.go	0.00%	19 Missing ⚠️
internal/pkg/platform/localserver.go	36.36%	7 Missing ⚠️
cmd/platform/run.go	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #378      +/-   ##
==========================================
+ Coverage   65.27%   65.43%   +0.16%     
==========================================
  Files         216      216              
  Lines       17994    17937      -57     
==========================================
- Hits        11745    11737       -8     
+ Misses       5160     5112      -48     
+ Partials     1089     1088       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mwbrooks

Some comments to connect the dots for the reviewers 🗺️ 📍

mwbrooks · 2026-03-10T16:34:01Z

cmd/platform/run.go

-		// OnEvent
-		func(event *logger.LogEvent) {
-			switch event.Name {
-			case "on_update_app_install":


note: This event was never used, disappointingly 😢

mwbrooks · 2026-03-10T16:35:06Z

cmd/platform/run.go

-					`Updating local app install for "%s"`,
-					event.DataToString("teamName"),
-				)))
-			case "on_cloud_run_connection_connected":


note: Moved here

mwbrooks · 2026-03-10T17:13:21Z

cmd/platform/run.go

-			case "on_cloud_run_connection_connected":
-				clients.IO.PrintTrace(ctx, slacktrace.PlatformRunReady)
-				cmd.Println(style.Secondary("Connected, awaiting events"))
-			case "on_cloud_run_connection_message":


note: Moved here

mwbrooks · 2026-03-10T17:14:14Z

cmd/platform/run.go

-			case "on_cloud_run_connection_message":
-				message := event.DataToString("cloud_run_connection_message")
-				clients.IO.PrintDebug(ctx, "received: %s", message)
-			case "on_cloud_run_connection_command_error":


note: Moved here

mwbrooks · 2026-03-10T17:14:41Z

cmd/platform/run.go

-			case "on_cloud_run_connection_command_error":
-				message := event.DataToString("cloud_run_connection_command_error")
-				clients.IO.PrintError(ctx, "Error: %s", message)
-			case "on_cloud_run_watch_error":


note: Moved to 3 places

mwbrooks · 2026-03-10T17:15:59Z

cmd/platform/run.go

-				cmd.Println(style.Secondary(fmt.Sprintf("Manifest change detected: %s, reinstalling app...", path)))
-			case "on_cloud_run_watch_manifest_change_reinstalled":
-				cmd.Println(style.Secondary("App successfully reinstalled"))
-			case "on_cloud_run_watch_manifest_change_skipped_remote":


note: Moved here

mwbrooks · 2026-03-10T17:16:20Z

cmd/platform/run.go

-			case "on_cloud_run_watch_manifest_change_skipped_remote":
-				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":


note: Moved here

mwbrooks · 2026-03-10T17:17:24Z

cmd/platform/run.go

-			case "on_cloud_run_watch_app_change":
-				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":


note: Moved here

mwbrooks · 2026-03-10T17:17:42Z

cmd/platform/run.go

-					`Cleaned up local app install for "%s".`,
-					event.DataToString("teamName"),
-				)))
-			case "on_cleanup_app_install_failed":


note: Moved here

mwbrooks · 2026-03-10T17:17:55Z

cmd/platform/run.go

-				)))
-				message := event.DataToString("on_cleanup_app_install_error")
-				clients.IO.PrintWarning(ctx, "Local app cleanup failed: %s", message)
-			case "on_abort_cleanup_app_install":


note: Never called

zimeg

@mwbrooks LGTM! Thanks for keeping these improvement chugging along 🌲 🚂 💨

zimeg · 2026-03-10T19:04:20Z

internal/pkg/platform/localserver.go

-				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)))


⭐ 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!

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.

zimeg · 2026-03-10T19:06:15Z

internal/pkg/platform/run.go

+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")
 }


💡 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 🎁

zimeg · 2026-03-10T19:06:48Z

internal/pkg/platform/run.go

 	variables, err := clients.Config.GetDotEnvFileVariables()
 	if err != nil {
-		return nil, "", slackerror.Wrap(err, slackerror.ErrLocalAppRun).
+		return "", slackerror.Wrap(err, slackerror.ErrLocalAppRun).


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

zimeg · 2026-03-10T19:16:44Z

internal/pkg/platform/localserver.go

 					break
 				}

-				r.log.Info("on_cloud_run_connection_command_output")


🪬 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.

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 🤪

mwbrooks · 2026-03-10T21:37:44Z

@zimeg Thanks for much for the review, again! 🙇🏻 Only one more PR left! 🤞🏻

mwbrooks added 6 commits March 5, 2026 14:52

refactor: remove logger from activity, validate, datastore, auth list…

83f47e1

…, delete, and uninstall

refactor: remove logger from project create

ff6c2c4

refactor: remove logger from app install, add, and function mocks

ea5b166

refactor: remove logger from platform run

a2b971d

Merge branch 'main' into mwbrooks-chopping-the-log-4

55066b9

# Conflicts: # cmd/project/create.go # cmd/project/create_test.go # internal/pkg/apps/install.go # internal/pkg/platform/localserver.go

fix: correct merge conflict resolutions for create.go and install.go

32a741d

mwbrooks added this to the Next Release milestone Mar 10, 2026

mwbrooks self-assigned this Mar 10, 2026

mwbrooks added code health M-T: Test improvements and anything that improves code health semver:patch Use on pull requests to describe the release version increment labels Mar 10, 2026

mwbrooks commented Mar 10, 2026

View reviewed changes

mwbrooks changed the title ~~refactor: remove logger from platform run~~ refactor: remove internal/logger from platform run Mar 10, 2026

mwbrooks marked this pull request as ready for review March 10, 2026 17:24

mwbrooks requested a review from a team as a code owner March 10, 2026 17:24

zimeg approved these changes Mar 10, 2026

View reviewed changes

mwbrooks merged commit 1ab5496 into main Mar 10, 2026
8 checks passed

mwbrooks deleted the mwbrooks-chopping-the-log-4 branch March 10, 2026 21:37

Conversation

mwbrooks commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog

Summary

Test Steps

Requirements

Uh oh!

codecov bot commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mwbrooks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zimeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwbrooks commented Mar 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mwbrooks commented Mar 10, 2026 •

edited

Loading

codecov bot commented Mar 10, 2026 •

edited

Loading