Skip to content

refactor: remove logger from install command #367

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.

Sign up for GitHub

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

Open
mwbrooks wants to merge 2 commits into
base: main
Choose a base branch
from
Open

refactor: remove logger from install command #367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b94ec1
refactor: remove logger from app install, add, and function mocks
mwbrooks Mar 6, 2026
2d3de52
refactor: fix formatting
mwbrooks Mar 6, 2026
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
68 changes: 4 additions & 64 deletions cmd/app/add.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,11 @@ package app

import (
"context"
"fmt"

"github.com/slackapi/slack-cli/internal/app"
"github.com/slackapi/slack-cli/internal/cmdutil"
"github.com/slackapi/slack-cli/internal/config"
"github.com/slackapi/slack-cli/internal/experiment"
"github.com/slackapi/slack-cli/internal/logger"
"github.com/slackapi/slack-cli/internal/pkg/apps"
"github.com/slackapi/slack-cli/internal/prompts"
"github.com/slackapi/slack-cli/internal/shared"
Expand Down Expand Up @@ -186,11 +184,8 @@ func RunAddCommand(ctx context.Context, clients *shared.ClientFactory, selection

clients.Config.ManifestEnv = app.SetManifestEnvTeamVars(clients.Config.ManifestEnv, selection.App.TeamDomain, selection.App.IsDev)

// Set up event logger
log := newAddLogger(clients, selection.Auth.TeamDomain)

// Install dev app or prod app to a workspace
installedApp, installState, err := appInstall(ctx, clients, log, selection, orgGrantWorkspaceID)
installedApp, installState, err := appInstall(ctx, clients, selection, orgGrantWorkspaceID)
if err != nil {
return ctx, installState, types.App{}, err // pass the installState because some callers may use it to handle the error
}
Expand All @@ -201,74 +196,19 @@ func RunAddCommand(ctx context.Context, clients *shared.ClientFactory, selection
return ctx, installState, installedApp, nil
}

// newAddLogger creates a logger instance to receive event notifications
func newAddLogger(clients *shared.ClientFactory, envName string) *logger.Logger {
return logger.New(
// OnEvent
func(event *logger.LogEvent) {
teamName := event.DataToString("teamName")
appName := event.DataToString("appName")
switch event.Name {
case "app_install_manifest":
// Ignore this event and format manifest outputs in create/update events
case "app_install_manifest_create":
Copy link
Member Author

@mwbrooks mwbrooks Mar 6, 2026

Choose a reason for hiding this comment

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

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

_, _ = clients.IO.WriteOut().Write([]byte(style.Sectionf(style.TextSection{
Emoji: "books",
Text: "App Manifest",
Secondary: []string{
fmt.Sprintf(`Creating app manifest for "%s" in "%s"`, appName, teamName),
},
})))
case "app_install_manifest_update":
_, _ = 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"`, appName, teamName),
},
})))
case "app_install_start":
_, _ = clients.IO.WriteOut().Write([]byte("\n" + style.Sectionf(style.TextSection{
Emoji: "house",
Text: "App Install",
Secondary: []string{
fmt.Sprintf(`Installing "%s" app to "%s"`, appName, teamName),
},
})))
case "app_install_icon_success":
iconPath := event.DataToString("iconPath")
_, _ = clients.IO.WriteOut().Write([]byte(
style.SectionSecondaryf("Updated app icon: %s", iconPath),
))
case "app_install_icon_error":
iconError := event.DataToString("iconError")
_, _ = clients.IO.WriteOut().Write([]byte(
style.SectionSecondaryf("Error updating app icon: %s", iconError),
))
case "app_install_complete":
_, _ = clients.IO.WriteOut().Write([]byte(
style.SectionSecondaryf("Finished in %s", event.DataToString("installTime")),
))
default:
// Ignore the event
}
},
)
}

// printAddSuccess will print a list of the environments
func printAddSuccess(clients *shared.ClientFactory, cmd *cobra.Command, appInstance types.App) error {
return runListCommand(cmd, clients)
}

// appInstall will install an app to a team. It supports both local and deployed app types.
func appInstall(ctx context.Context, clients *shared.ClientFactory, log *logger.Logger, selection *prompts.SelectedApp, orgGrantWorkspaceID string) (types.App, types.InstallState, error) {
func appInstall(ctx context.Context, clients *shared.ClientFactory, selection *prompts.SelectedApp, orgGrantWorkspaceID string) (types.App, types.InstallState, error) {
if selection != nil && selection.App.IsDev {
// Install local dev app to a team
installedApp, _, installState, err := appInstallDevAppFunc(ctx, clients, "", log, selection.Auth, selection.App)
installedApp, _, installState, err := appInstallDevAppFunc(ctx, clients, "", selection.Auth, selection.App)
return installedApp, installState, err
} else {
installState, installedApp, err := appInstallProdAppFunc(ctx, clients, log, selection.Auth, selection.App, orgGrantWorkspaceID)
installState, installedApp, err := appInstallProdAppFunc(ctx, clients, selection.Auth, selection.App, orgGrantWorkspaceID)
return installedApp, installState, err
}
}
9 changes: 4 additions & 5 deletions cmd/function/mocks_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ package function_test
import (
"context"

"github.com/slackapi/slack-cli/internal/logger"
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/shared/types"
"github.com/stretchr/testify/mock"
Expand All @@ -27,22 +26,22 @@ type FunctionDistributorMock struct {
mock.Mock
}

func (m *FunctionDistributorMock) List(ctx context.Context, clients *shared.ClientFactory, fn string, log *logger.Logger) (types.Permission, []types.FunctionDistributionUser, error) {
func (m *FunctionDistributorMock) List(ctx context.Context, clients *shared.ClientFactory, fn string) (types.Permission, []types.FunctionDistributionUser, error) {
args := m.Called()
return args.Get(0).(types.Permission), args.Get(1).([]types.FunctionDistributionUser), args.Error(2)
}

func (m *FunctionDistributorMock) Set(ctx context.Context, clients *shared.ClientFactory, function, distributionType, users string, log *logger.Logger) (string, error) {
func (m *FunctionDistributorMock) Set(ctx context.Context, clients *shared.ClientFactory, function, distributionType, users string) (string, error) {
args := m.Called()
return args.String(0), args.Error(1)
}

func (m *FunctionDistributorMock) AddUsers(ctx context.Context, clients *shared.ClientFactory, function, users string, log *logger.Logger) (string, error) {
func (m *FunctionDistributorMock) AddUsers(ctx context.Context, clients *shared.ClientFactory, function, users string) (string, error) {
args := m.Called()
return args.String(0), args.Error(1)
}

func (m *FunctionDistributorMock) RemoveUsers(ctx context.Context, clients *shared.ClientFactory, function, users string, log *logger.Logger) (string, error) {
func (m *FunctionDistributorMock) RemoveUsers(ctx context.Context, clients *shared.ClientFactory, function, users string) (string, error) {
args := m.Called()
return args.String(0), args.Error(1)
}
24 changes: 7 additions & 17 deletions internal/pkg/apps/add.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,35 +19,31 @@ import (
"strings"

"github.com/opentracing/opentracing-go"
"github.com/slackapi/slack-cli/internal/logger"
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/shared/types"
"github.com/slackapi/slack-cli/internal/slackerror"
)

// Add will add an app
func Add(ctx context.Context, clients *shared.ClientFactory, log *logger.Logger, auth types.SlackAuth, app types.App, orgGrantWorkspaceID string) (types.InstallState, types.App, error) {
func Add(ctx context.Context, clients *shared.ClientFactory, auth types.SlackAuth, app types.App, orgGrantWorkspaceID string) (types.InstallState, types.App, error) {
span, _ := opentracing.StartSpanFromContext(ctx, "pkg.apps.add")
defer span.Finish()

// Validate the auth
ctx, authSession, err := getAuthSession(ctx, clients, auth)
ctx, _, err := getAuthSession(ctx, clients, auth)
if err != nil {
return "", types.App{}, slackerror.Wrap(err, slackerror.ErrAddAppToProject)
}
log.Data["teamName"] = *authSession.TeamName

log.Info("on_apps_add_init")
Copy link
Member Author

@mwbrooks mwbrooks Mar 6, 2026

Choose a reason for hiding this comment

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

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


// Add app remotely via Slack API
installState, app, err := addAppRemotely(ctx, clients, log, auth, app, orgGrantWorkspaceID)
installState, app, err := addAppRemotely(ctx, clients, auth, app, orgGrantWorkspaceID)
if err != nil {
return "", types.App{}, slackerror.Wrap(err, slackerror.ErrAddAppToProject)
}

// Add app to apps.json
if !clients.Config.SkipLocalFs() {
if _, err := addAppLocally(ctx, clients, log, app); err != nil {
if _, err := addAppLocally(ctx, clients, app); err != nil {
return installState, types.App{}, slackerror.Wrap(err, slackerror.ErrAddAppToProject)
}
}
Expand All @@ -56,9 +52,7 @@ func Add(ctx context.Context, clients *shared.ClientFactory, log *logger.Logger,
}

// 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) {
Copy link
Member Author

@mwbrooks mwbrooks Mar 6, 2026

Choose a reason for hiding this comment

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

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

log.Info("on_apps_add_local")
Copy link
Member Author

@mwbrooks mwbrooks Mar 6, 2026

Choose a reason for hiding this comment

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

note: Another event that was broadcast with no subscribers.


func addAppLocally(ctx context.Context, clients *shared.ClientFactory, app types.App) (types.App, error) {
app, err := clients.AppClient().NewDeployed(ctx, app.TeamID)
if err != nil {
if !strings.Contains(err.Error(), slackerror.ErrAppFound) { // Ignore the error when the app already exists
Expand All @@ -69,22 +63,18 @@ func addAppLocally(ctx context.Context, clients *shared.ClientFactory, log *logg
}

// addAppRemotely will create the app manifest using the current auth account's team
func addAppRemotely(ctx context.Context, clients *shared.ClientFactory, log *logger.Logger, auth types.SlackAuth, app types.App, orgGrantWorkspaceID string) (types.InstallState, types.App, error) {
log.Info("on_apps_add_remote_init")

func addAppRemotely(ctx context.Context, clients *shared.ClientFactory, auth types.SlackAuth, app types.App, orgGrantWorkspaceID string) (types.InstallState, types.App, error) {
if app.TeamID == "" {
// App hasn't been created yet and
// so the target team ID set by the auth
app.TeamID = auth.TeamID
}

app, installState, err := Install(ctx, clients, log, auth, CreateAppManifestAndInstall, app, orgGrantWorkspaceID)
app, installState, err := Install(ctx, clients, auth, CreateAppManifestAndInstall, app, orgGrantWorkspaceID)
if err != nil {
return installState, types.App{}, slackerror.Wrap(err, slackerror.ErrAppAdd)
}

log.Info("on_apps_add_remote_success")
Copy link
Member Author

@mwbrooks mwbrooks Mar 6, 2026

Choose a reason for hiding this comment

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

note: another event that was broadcast with no subscribers 😿


if !clients.Config.SkipLocalFs() {
app, err = clients.AppClient().GetDeployed(ctx, app.TeamID)
}
Expand Down
Loading
Loading