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

mwbrooks · 2026-03-10T23:21:16Z

Changelog

N/A

Summary

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

Package and Deploy Spinner logic moved to internal/pkg/platform/deploy.go
- This is a step backwards in where we want to go
- But it's the cleanest solution that consolidates the spinner to a single package
- Hopefully it makes future refactors that move display logic to cmd/ more straight-forward

Preview

Note: Previews use an artificially long packaging time, so that you can see the spinner.

1. Deno + ROSI Deploy

2026-03-10-deno-deploy.mov

2. Deno + ROSI Re-Deploy

2026-03-10-deno-deploy-again.mov

3. Bolt + Deploy Script

2026-03-10-bolt-deploy.mov

Test Steps

There are 3 test scenarios:

1. Deno + ROSI Deploy

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

# Deploy
$ lack deploy
# → Confirm output and formatting look correct

# Cleanup in Test 2

2. Deno + ROSI Re-Deploy

# Based on Test 1

# Re-Deploy
$ lack deploy

# Cleanup
$ lack delete -f
$ cd ..
$ rm -rf my-app

3. Bolt + Deploy Script

# Create Bolt project
$ lack create my-app -t https://github.com/slack-samples/bolt-js-starter-template
$ cd my-app/

# Deploy
$ lack deploy
# → Confirm output and formatting look correct

# Cleanup
$ 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.

…er package

mwbrooks

Comments to guide the reviewers through this phase 🪵 🪓 of package logger

mwbrooks · 2026-03-10T23:23:35Z

cmd/platform/deploy.go

-			case "on_app_package":
-				printDeployPackage(cmd, event)


note: Started the packaging spinner.

mwbrooks · 2026-03-10T23:23:43Z

cmd/platform/deploy.go

-			case "on_app_package_completion":
-				printDeployPackageCompletion(cmd, event)


note: Stopped the packaging spinner.

mwbrooks · 2026-03-10T23:24:19Z

cmd/platform/deploy.go

-			case "on_app_deploy_hosting":
-				printDeployHosting(cmd, event)


note: Started the deploy spinner. Stopping the spinner was handled by the print complete function after the command completes.

mwbrooks · 2026-03-10T23:28:52Z

cmd/platform/deploy.go

-func printDeployPackage(cmd *cobra.Command, event *logger.LogEvent) {
-	cmd.Println()
-	packageSpinner.Update("Packaging app for deployment", "").Start()
-}


note: Moved

mwbrooks · 2026-03-10T23:29:29Z

cmd/platform/deploy.go

-func printDeployPackageCompletion(cmd *cobra.Command, event *logger.LogEvent) {
-	packagedSize := event.DataToString("packagedSize")
-	packagedTime := event.DataToString("packagedTime")
-
-	deployPackageSuccessText := style.Sectionf(style.TextSection{
-		Emoji:     "gift",
-		Text:      "App packaged and ready to deploy",
-		Secondary: []string{fmt.Sprintf("%s was packaged in %s", packagedSize, packagedTime)},
-	})
-	packageSpinner.Update(deployPackageSuccessText, "").Stop()
-}


note: Moved

mwbrooks · 2026-03-10T23:29:55Z

cmd/platform/deploy.go

-func printDeployHosting(cmd *cobra.Command, event *logger.LogEvent) {
-	deployHostingText := "Deploying to Slack Platform" + style.Secondary(" (this will take a moment)")
-	deploySpinner.Update(deployHostingText, "").Start()


note: Moved

🪓 praise: These are nice functions to be removing with these change!

mwbrooks · 2026-03-10T23:31:41Z

cmd/platform/deploy.go

-		Secondary: []string{style.Mapf(parsedAppInfo)},
-	})
-
-	deploySpinner.Update(successfulDeployText, "").Stop()


note: Moved

mwbrooks · 2026-03-10T23:33:16Z

cmd/platform/deploy_test.go

-		event    logger.LogData
-		expected []string
-	}{
-		"information from a workspace deploy is printed": {


note: These 3 tests are moved to internal/pkg/platform/deploy_test.go

🪬 question: In later changes do you think testing outputs remain useful? I understand this guards against regression but the slacktrace package might have nice patterns to support instead of "info" prints?

That's something to think about. A few reason to test outputs might be:

Outputs can be different based on scenarios: Enterprise vs Workspace for this PR

TTY vs Non-TTY

Confirming debug logs or error logs are printed

Logs are important for... logging (logstash), so we'd want a few gut checks for sure

I think the main concern is brittle test based on specific text and redundant tests. So, we could ask ourselves how often we'd hit this situation.

mwbrooks · 2026-03-10T23:34:30Z

internal/logger/logger.go

-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package logger


mwbrooks · 2026-03-10T23:50:30Z

internal/pkg/platform/deploy.go

 }

-func deployApp(ctx context.Context, clients *shared.ClientFactory, log *logger.Logger, app types.App) (types.App, error) {
+func deployApp(ctx context.Context, clients *shared.ClientFactory, app types.App, manifest types.SlackYaml, authSession api.AuthSession) error {


note: Not ideal, but we now need to pass in the app manifest and auth session because we output the info in this method (it's where the spinner now resides).

👁️‍🗨️ thought: These are meaningful values to pass to the "deploy" function IMO! But I'm realizing it isn't clear that these values aren't used for more than outputs. Still meaningful to me!

Continuing your thought: I often feel like slackcontext could be used here. slackcontext.App(ctx) where App is a more complete struct that contains the App, User, and Team with rich metadata (not just IDs).

codecov · 2026-03-11T02:11:03Z

Codecov Report

❌ Patch coverage is 43.05556% with 41 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.40%. Comparing base (1ab5496) to head (22f41d8).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/pkg/platform/deploy.go	36.92%	41 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #384      +/-   ##
==========================================
- Coverage   65.42%   65.40%   -0.03%     
==========================================
  Files         216      214       -2     
  Lines       17937    17847      -90     
==========================================
- Hits        11736    11672      -64     
+ Misses       5111     5087      -24     
+ Partials     1090     1088       -2

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

zimeg

@mwbrooks Super awesome sets of change across #364 #366 #367 #378!

🪓 ✨ IIRC this makes -990 lines altogether and more simple logic to skim I think! Thanks for taking the time to make these changes sincere. I'm leaving a few comments with thoughts but think this can merge whenever is best!

zimeg · 2026-03-11T02:55:29Z

internal/pkg/platform/deploy_test.go

+func strPtr(s string) *string { return &s }
+func boolPtr(b bool) *bool    { return &b }


🔍 question(non-blocking): Can we use the new constructor instead of these functions? I haven't explored this but it seems, new, to the latest releases:

The built-in new function, which creates a new variable, now allows its operand to be an expression, specifying the initial value of the variable.

📚 https://go.dev/doc/go1.26#language

🧠 ✨ @zimeg Wow, you're staying current with the absolute latest features! This is a great suggestion and helps to remove the awkward pointer constructors! Commit 22f41d8

zimeg · 2026-03-11T02:56:51Z

internal/logger/logger.go

-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package logger


zimeg · 2026-03-11T03:02:35Z

cmd/platform/deploy_test.go

-		event    logger.LogData
-		expected []string
-	}{
-		"information from a workspace deploy is printed": {


🪬 question: In later changes do you think testing outputs remain useful? I understand this guards against regression but the slacktrace package might have nice patterns to support instead of "info" prints?

zimeg · 2026-03-11T03:08:44Z

cmd/platform/deploy.go

-var packageSpinner *style.Spinner
-var deploySpinner *style.Spinner


🌟 praise: These variables have been confusing to reason about across package log events in earlier changes I found! Thanks for finding a nice pattern for these!

zimeg · 2026-03-11T03:10:04Z

cmd/platform/deploy.go

-func printDeployHosting(cmd *cobra.Command, event *logger.LogEvent) {
-	deployHostingText := "Deploying to Slack Platform" + style.Secondary(" (this will take a moment)")
-	deploySpinner.Update(deployHostingText, "").Start()


🪓 praise: These are nice functions to be removing with these change!

zimeg · 2026-03-11T03:13:59Z

internal/pkg/platform/deploy.go

 }

-func deployApp(ctx context.Context, clients *shared.ClientFactory, log *logger.Logger, app types.App) (types.App, error) {
+func deployApp(ctx context.Context, clients *shared.ClientFactory, app types.App, manifest types.SlackYaml, authSession api.AuthSession) error {


👁️‍🗨️ thought: These are meaningful values to pass to the "deploy" function IMO! But I'm realizing it isn't clear that these values aren't used for more than outputs. Still meaningful to me!

mwbrooks · 2026-03-11T04:13:59Z

@zimeg Thanks for the quick review! This brings us to the end of removing the package logger. We may find the odd quirk from the refactor, but I think we've approached this as carefully as we could. Thanks for the reviews along the way! 🙇🏻

mwbrooks added 3 commits March 10, 2026 14:40

refactor: remove logger from platform deploy and delete internal/logg…

de7cc3f

…er package

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

7ee1271

refactor: fix package and deploy spinner

bee70ec

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 added 6 commits March 10, 2026 16:25

refactor: remove outdate FIXME

8338b90

refactor: remove unused result from deployHook func

19f6896

refactor: remove DeployResult from Deploy func

d37b869

refactor: remove unused return args from deployApp func

34a46e4

refactor: minor alignments with original source code

3376ef2

refactor: remove unused sleep time to test packaging

296c3bd

mwbrooks commented Mar 11, 2026

View reviewed changes

mwbrooks marked this pull request as ready for review March 11, 2026 00:17

mwbrooks requested a review from a team as a code owner March 11, 2026 00:17

zimeg approved these changes Mar 11, 2026

View reviewed changes

refactor: replace strPtr and boolPtr with new-new constructor

22f41d8

mwbrooks merged commit d1f659d into main Mar 11, 2026
8 checks passed

mwbrooks deleted the mwbrooks-chopping-the-log-5 branch March 11, 2026 04:14

		case "on_app_package_completion":
		printDeployPackageCompletion(cmd, event)

		func strPtr(s string) *string { return &s }
		func boolPtr(b bool) *bool { return &b }

		var packageSpinner *style.Spinner
		var deploySpinner *style.Spinner

Conversation

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

Uh oh!

Changelog

Summary

Preview

1. Deno + ROSI Deploy

2. Deno + ROSI Re-Deploy

3. Bolt + Deploy Script

Test Steps

1. Deno + ROSI Deploy

2. Deno + ROSI Re-Deploy

3. Bolt + Deploy Script

Requirements

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!

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 Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

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!

Choose a reason for hiding this comment

Uh oh!

mwbrooks commented Mar 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

mwbrooks commented Mar 10, 2026 •

edited

Loading

mwbrooks Mar 11, 2026 •

edited

Loading

codecov bot commented Mar 11, 2026 •

edited

Loading