refactor: remove logger from install command by mwbrooks · Pull Request #367 · slackapi/slack-cli

mwbrooks · 2026-03-06T23:07:03Z

Changelog

N/A

Summary

This pull request is part 3 of removing internal/logger package and is focused on the the install command.

Impacts Deno deployed installations
Impacts Bolt or local installations

Test Steps

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

# TEST: Deploy
$ lack deploy
# → Confirm no unexpected newlines or spacing issues
# → Note: Trigger creation has a double newline in production

# TEST: Re-Deploy
$ lack deploy
# → Confirm no unexpected newlines or spacing issues
# → Note: Trigger creation has a double newline in production
# → Note: Manifest warning home_team_only has a double newline in production

# TEST: Run
$ lack run
# → Confirm no unexpected newlines or spacing issues

# TEST: Re-Run
$ lack run
# → Confirm no unexpected newlines or spacing issues
# → Note: Manifest warning home_team_only has a double newline in production

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

codecov · 2026-03-06T23:12:51Z

Codecov Report

❌ Patch coverage is 81.96721% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.11%. Comparing base (c449532) to head (2d3de52).

Files with missing lines	Patch %	Lines
internal/pkg/apps/add.go	0.00%	7 Missing ⚠️
internal/pkg/apps/install.go	95.83%	2 Missing ⚠️
internal/pkg/platform/localserver.go	0.00%	1 Missing ⚠️
internal/pkg/platform/run.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #367      +/-   ##
==========================================
+ Coverage   65.06%   65.11%   +0.04%     
==========================================
  Files         215      215              
  Lines       18179    18144      -35     
==========================================
- Hits        11829    11815      -14     
+ Misses       5254     5235      -19     
+ Partials     1096     1094       -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.

mwbrooks

Comments for the reviewers eyes

mwbrooks · 2026-03-06T23:08:38Z

cmd/app/add.go

-			switch event.Name {
-			case "app_install_manifest":
-				// Ignore this event and format manifest outputs in create/update events
-			case "app_install_manifest_create":


note: You can search for any of the following event names to see that the output is now display where the event was previous broadcast:

app_install_manifest_create

app_install_manifest_update

app_install_start

app_install_icon_success

app_install_icon_error

app_install_complete

mwbrooks · 2026-03-06T23:09:47Z

internal/pkg/apps/add.go

 	}
-	log.Data["teamName"] = *authSession.TeamName
-
-	log.Info("on_apps_add_init")


note: This event name was broadcast but never subscribed anywhere 🤦🏻

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

internal/pkg/apps/add.go

 }

 // addAppLocally will add the app to the project's apps file with an empty AppID, TeamID, etc
-func addAppLocally(ctx context.Context, clients *shared.ClientFactory, log *logger.Logger, app types.App) (types.App, error) {


note: diff doesn't show that log *logger.Logger was removed.

mwbrooks · 2026-03-06T23:10:44Z

internal/pkg/apps/add.go


 // addAppLocally will add the app to the project's apps file with an empty AppID, TeamID, etc
-func addAppLocally(ctx context.Context, clients *shared.ClientFactory, log *logger.Logger, app types.App) (types.App, error) {
-	log.Info("on_apps_add_local")


note: Another event that was broadcast with no subscribers.

mwbrooks · 2026-03-06T23:11:19Z

internal/pkg/apps/add.go

 		return installState, types.App{}, slackerror.Wrap(err, slackerror.ErrAppAdd)
 	}

-	log.Info("on_apps_add_remote_success")


note: another event that was broadcast with no subscribers 😿

mwbrooks · 2026-03-06T23:11:55Z

internal/pkg/apps/install.go

+		clients.IO.PrintInfo(ctx, false, "\n%s", style.Sectionf(style.TextSection{
+			Emoji: "books",
+			Text:  "App Manifest",
+			Secondary: []string{
+				fmt.Sprintf(`Updated app manifest for "%s" in "%s"`, slackManifest.DisplayInformation.Name, *authSession.TeamName),
+			},
+		}))


note: Example of replacing the event broadcast with the actual output.

mwbrooks · 2026-03-06T23:12:46Z

internal/pkg/apps/install.go

-	appManageURL := fmt.Sprintf("%s/apps", apiInterface.Host())
-	log.Data["appURL"] = fmt.Sprintf("%s%s", appManageURL, app.AppID)
-	log.Data["appName"] = manifest.DisplayInformation.Name


note: appURL was never output anywhere.

mwbrooks · 2026-03-06T23:57:48Z

internal/pkg/apps/install.go

+		_, _ = clients.IO.WriteOut().Write([]byte("\n" + style.Sectionf(style.TextSection{
+			Emoji: "books",
+			Text:  "App Manifest",
+			Secondary: []string{
+				fmt.Sprintf(`Updated app manifest for "%s" in "%s"`, slackManifest.DisplayInformation.Name, *authSession.TeamName),
+			},
+		})))


note: I'm using the identical clients.IO.WriteOut() syntax from cmd/ to avoid introducing formatting issues. The clients.IO.PrintInfo inserts newlines after the output, causing issues.

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

1b94ec1

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

mwbrooks self-assigned this Mar 6, 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 6, 2026

mwbrooks commented Mar 6, 2026

View reviewed changes

mwbrooks changed the title ~~refactor: remove logger from app install, add, and function mocks~~ refactor: remove logger from install command Mar 6, 2026

refactor: fix formatting

2d3de52

mwbrooks commented Mar 7, 2026

View reviewed changes

mwbrooks marked this pull request as ready for review March 7, 2026 00:08

mwbrooks requested a review from a team as a code owner March 7, 2026 00:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: remove logger from install command#367

refactor: remove logger from install command#367
mwbrooks wants to merge 2 commits intomainfrom
mwbrooks-chopping-the-log-3

mwbrooks commented Mar 6, 2026 •

edited

Loading

Uh oh!

codecov bot commented Mar 6, 2026 •

edited

Loading

Uh oh!

mwbrooks left a comment

Uh oh!

mwbrooks Mar 6, 2026

Uh oh!

mwbrooks Mar 6, 2026

Uh oh!

mwbrooks Mar 6, 2026

Uh oh!

mwbrooks Mar 6, 2026

Uh oh!

mwbrooks Mar 6, 2026

Uh oh!

mwbrooks Mar 6, 2026

Uh oh!

mwbrooks Mar 6, 2026

Uh oh!

mwbrooks Mar 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

mwbrooks commented Mar 6, 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 6, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

mwbrooks commented Mar 6, 2026 •

edited

Loading

codecov bot commented Mar 6, 2026 •

edited

Loading