Skip to content

refactor: remove logger from activity, manifest, datastore, auth, and app delete/uninstall #364

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 1 commit into
base: main
Choose a base branch
from
Open

refactor: remove logger from activity, manifest, datastore, auth, and app delete/uninstall #364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
31 changes: 7 additions & 24 deletions cmd/app/delete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (

"github.com/slackapi/slack-cli/internal/cmdutil"
"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/prompts"
"github.com/slackapi/slack-cli/internal/shared"
Expand Down Expand Up @@ -99,30 +98,14 @@ func RunDeleteCommand(ctx context.Context, clients *shared.ClientFactory, cmd *c
}
}

// Set up event logger and execute the command
log := newDeleteLogger(clients, cmd, team)
log.Data["appID"] = selection.App.AppID
env, err := apps.Delete(ctx, clients, log, team, selection.App, selection.Auth)

return env, err
}
// Execute the command
env, teamName, err := apps.Delete(ctx, clients, team, selection.App, selection.Auth)
if err != nil {
return env, err
}
printDeleteApp(ctx, clients, selection.App.AppID, teamName)

// newDeleteLogger creates a logger instance to receive event notifications
func newDeleteLogger(clients *shared.ClientFactory, cmd *cobra.Command, envName string) *logger.Logger {
ctx := cmd.Context()
return logger.New(
// OnEvent
func(event *logger.LogEvent) {
appID := event.DataToString("appID")
teamName := event.DataToString("teamName")
switch event.Name {
case "on_apps_delete_app_success":
printDeleteApp(ctx, clients, appID, teamName)
default:
// Ignore the event
}
},
)
Comment on lines -102 to -125
Copy link
Member Author

@mwbrooks mwbrooks Mar 5, 2026

Choose a reason for hiding this comment

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

note: This is a good example of a typical change.

return env, nil
}

func confirmDeletion(ctx context.Context, IO iostreams.IOStreamer, app prompts.SelectedApp) (bool, error) {
Expand Down
31 changes: 7 additions & 24 deletions cmd/app/uninstall.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (

"github.com/slackapi/slack-cli/internal/cmdutil"
"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/prompts"
"github.com/slackapi/slack-cli/internal/shared"
Expand Down Expand Up @@ -94,30 +93,14 @@ func RunUninstallCommand(ctx context.Context, clients *shared.ClientFactory, cmd
}
}

// Set up event logger and execute the command
log := newUninstallLogger(clients, cmd, teamDomain)
log.Data["appID"] = selection.App.AppID
env, err := apps.Uninstall(ctx, clients, log, teamDomain, selection.App, selection.Auth)

return env, err
}
// Execute the command
env, teamName, err := apps.Uninstall(ctx, clients, teamDomain, selection.App, selection.Auth)
if err != nil {
return env, err
}
printUninstallApp(ctx, clients, selection.App.AppID, teamName)

// newUninstallLogger creates a logger instance to receive event notifications
func newUninstallLogger(clients *shared.ClientFactory, cmd *cobra.Command, envName string) *logger.Logger {
ctx := cmd.Context()
return logger.New(
// OnEvent
func(event *logger.LogEvent) {
appID := event.DataToString("appID")
teamName := event.DataToString("teamName")
switch event.Name {
case "on_apps_uninstall_app_success":
printUninstallApp(ctx, clients, appID, teamName)
default:
// Ignore the event
}
},
)
return env, nil
}

func confirmUninstall(ctx context.Context, IO iostreams.IOStreamer, cmd *cobra.Command, selection prompts.SelectedApp) (bool, error) {
Expand Down
3 changes: 1 addition & 2 deletions cmd/auth/auth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
"testing"

"github.com/slackapi/slack-cli/internal/hooks"
"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/slackcontext"
Expand All @@ -32,7 +31,7 @@ type listMockObject struct {
mock.Mock
}

func (m *listMockObject) MockListFunction(ctx context.Context, clients *shared.ClientFactory, log *logger.Logger) ([]types.SlackAuth, error) {
func (m *listMockObject) MockListFunction(ctx context.Context, clients *shared.ClientFactory) ([]types.SlackAuth, error) {
args := m.Called()
return args.Get(0).([]types.SlackAuth), args.Error(1)
}
Expand Down
24 changes: 2 additions & 22 deletions cmd/auth/list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
"fmt"

"github.com/slackapi/slack-cli/internal/iostreams"
"github.com/slackapi/slack-cli/internal/logger"
"github.com/slackapi/slack-cli/internal/pkg/auth"
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/shared/types"
Expand Down Expand Up @@ -52,34 +51,15 @@ func NewListCommand(clients *shared.ClientFactory) *cobra.Command {
// runListCommand will execute the list command
func runListCommand(cmd *cobra.Command, clients *shared.ClientFactory) error {
ctx := cmd.Context()
log := newListLogger(cmd, clients.IO)
userAuthList, err := listFunc(ctx, clients, log)
userAuthList, err := listFunc(ctx, clients)
if err != nil {
return err
}
printAuthList(cmd, clients.IO, userAuthList)
printAuthListSuccess(cmd, clients.IO, userAuthList)
return nil
}

// newListLogger creates a logger instance to receive event notifications
func newListLogger(cmd *cobra.Command, IO iostreams.IOStreamer) *logger.Logger {
return logger.New(
// OnEvent
func(event *logger.LogEvent) {
switch event.Name {
case "on_auth_list":
userAuthList := []types.SlackAuth{}
if event.Data["userAuthList"] != nil {
userAuthList = event.Data["userAuthList"].([]types.SlackAuth)
}
printAuthList(cmd, IO, userAuthList)
default:
// Ignore the event
}
},
)
}

// printAuthList will display a list of all authorizations available and highlight the default.
// The output will be a list formatted as:
//
Expand Down
3 changes: 1 addition & 2 deletions cmd/auth/list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
"context"
"testing"

"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/slackcontext"
Expand All @@ -32,7 +31,7 @@ type ListPkgMock struct {
mock.Mock
}

func (m *ListPkgMock) List(ctx context.Context, clients *shared.ClientFactory, log *logger.Logger) ([]types.SlackAuth, error) {
func (m *ListPkgMock) List(ctx context.Context, clients *shared.ClientFactory) ([]types.SlackAuth, error) {
m.Called()
return []types.SlackAuth{}, nil
}
Expand Down
29 changes: 4 additions & 25 deletions cmd/datastore/bulk_delete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"github.com/slackapi/slack-cli/internal/cmdutil"
"github.com/slackapi/slack-cli/internal/config"
"github.com/slackapi/slack-cli/internal/goutils"
"github.com/slackapi/slack-cli/internal/logger"
"github.com/slackapi/slack-cli/internal/pkg/datastore"
"github.com/slackapi/slack-cli/internal/prompts"
"github.com/slackapi/slack-cli/internal/shared"
Expand Down Expand Up @@ -88,12 +87,12 @@ func NewBulkDeleteCommand(clients *shared.ClientFactory) *cobra.Command {
}

// Perform the delete
log := newBulkDeleteLogger(clients, cmd)
event, err := BulkDelete(ctx, clients, log, query)
result, err := BulkDelete(ctx, clients, query)
if err != nil {
return err
}
printDatastoreBulkDeleteSuccess(cmd, event)
_ = printBulkDeleteResult(clients, cmd, result)
printDatastoreBulkDeleteSuccess(cmd)
return nil
},
}
Expand All @@ -118,26 +117,6 @@ func preRunBulkDeleteCommandFunc(ctx context.Context, clients *shared.ClientFact
return cmdutil.IsSlackHostedProject(ctx, clients)
}

func newBulkDeleteLogger(clients *shared.ClientFactory, cmd *cobra.Command) *logger.Logger {
return logger.New(
// OnEvent
func(event *logger.LogEvent) {
switch event.Name {
case "on_bulk_delete_result":
deleteResult := types.AppDatastoreBulkDeleteResult{}
if event.Data["bulkDeleteResult"] != nil {
deleteResult = event.Data["bulkDeleteResult"].(types.AppDatastoreBulkDeleteResult)
}
if cmd != nil {
_ = printBulkDeleteResult(clients, cmd, deleteResult)
}
default:
// Ignore the event
}
},
)
}

func printBulkDeleteResult(clients *shared.ClientFactory, cmd *cobra.Command, deleteResult types.AppDatastoreBulkDeleteResult) error {
var datastore = deleteResult.Datastore
cmd.Printf(
Expand Down Expand Up @@ -166,7 +145,7 @@ func printBulkDeleteResult(clients *shared.ClientFactory, cmd *cobra.Command, de
return nil
}

func printDatastoreBulkDeleteSuccess(cmd *cobra.Command, event *logger.LogEvent) {
func printDatastoreBulkDeleteSuccess(cmd *cobra.Command) {
commandText := style.Commandf("datastore query <expression>", true)
if cmd != nil {
cmd.Printf(
Expand Down
13 changes: 5 additions & 8 deletions cmd/datastore/bulk_delete_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (

"github.com/slackapi/slack-cli/internal/app"
"github.com/slackapi/slack-cli/internal/config"
"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/slackcontext"
Expand All @@ -36,11 +35,9 @@ type BulkDeletePkgMock struct {
mock.Mock
}

func (m *BulkDeletePkgMock) BulkDelete(ctx context.Context, clients *shared.ClientFactory, log *logger.Logger, query types.AppDatastoreBulkDelete) (*logger.LogEvent, error) {
m.Called(ctx, clients, log, query)
log.Data["bulkDeleteResult"] = types.AppDatastoreBulkDeleteResult{}
log.Log("info", "on_bulk_delete_result")
return log.SuccessEvent(), nil
func (m *BulkDeletePkgMock) BulkDelete(ctx context.Context, clients *shared.ClientFactory, query types.AppDatastoreBulkDelete) (types.AppDatastoreBulkDeleteResult, error) {
m.Called(ctx, clients, query)
return types.AppDatastoreBulkDeleteResult{}, nil
}

func TestBulkDeleteCommandPreRun(t *testing.T) {
Expand Down Expand Up @@ -185,7 +182,7 @@ func TestBulkDeleteCommand(t *testing.T) {
// Create mocked command
bulkDeleteMock := new(BulkDeletePkgMock)
BulkDelete = bulkDeleteMock.BulkDelete
bulkDeleteMock.On("BulkDelete", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
bulkDeleteMock.On("BulkDelete", mock.Anything, mock.Anything, mock.Anything).Return(types.AppDatastoreBulkDeleteResult{}, nil)

cmd := NewBulkDeleteCommand(clients)
// TODO: could maybe refactor this to the os/fs mocks level to more clearly communicate "fake being in an app directory"
Expand All @@ -198,7 +195,7 @@ func TestBulkDeleteCommand(t *testing.T) {
// Create mocked command
err := cmd.ExecuteContext(ctx)
if assert.NoError(t, err) {
bulkDeleteMock.AssertCalled(t, "BulkDelete", mock.Anything, mock.Anything, mock.Anything, tc.Query)
bulkDeleteMock.AssertCalled(t, "BulkDelete", mock.Anything, mock.Anything, tc.Query)
}

// Cleanup when done
Expand Down
30 changes: 4 additions & 26 deletions cmd/datastore/bulk_get.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"github.com/slackapi/slack-cli/internal/cmdutil"
"github.com/slackapi/slack-cli/internal/config"
"github.com/slackapi/slack-cli/internal/goutils"
"github.com/slackapi/slack-cli/internal/logger"
"github.com/slackapi/slack-cli/internal/pkg/datastore"
"github.com/slackapi/slack-cli/internal/prompts"
"github.com/slackapi/slack-cli/internal/shared"
Expand Down Expand Up @@ -90,12 +89,12 @@ func NewBulkGetCommand(clients *shared.ClientFactory) *cobra.Command {
}

// Perform the get
log := newBulkGetLogger(clients, cmd, query)
event, err := BulkGet(ctx, clients, log, query)
result, err := BulkGet(ctx, clients, query)
if err != nil {
return err
}
printDatastoreBulkGetSuccess(cmd, event)
_ = printBulkGetResult(clients, cmd, query, result)
printDatastoreBulkGetSuccess(cmd)
return nil
},
}
Expand All @@ -121,27 +120,6 @@ func preRunBulkGetCommandFunc(ctx context.Context, clients *shared.ClientFactory
return cmdutil.IsSlackHostedProject(ctx, clients)
}

func newBulkGetLogger(clients *shared.ClientFactory, cmd *cobra.Command, request types.AppDatastoreBulkGet) *logger.Logger {
return logger.New(
// OnEvent
func(event *logger.LogEvent) {
switch event.Name {
case "on_bulk_get_result":
getResult := types.AppDatastoreBulkGetResult{}
if event.Data["bulkGetResult"] != nil {
getResult = event.Data["bulkGetResult"].(types.AppDatastoreBulkGetResult)
}
if cmd != nil {
// TODO: this can raise an error on indentation failures, but not sure how to handle that using logger
_ = printBulkGetResult(clients, cmd, request, getResult)
}
default:
// Ignore the event
}
},
)
}

func printBulkGetResult(clients *shared.ClientFactory, cmd *cobra.Command, request types.AppDatastoreBulkGet, getResult types.AppDatastoreBulkGetResult) error {
var datastore = getResult.Datastore
var items = getResult.Items
Expand Down Expand Up @@ -189,5 +167,5 @@ func printBulkGetResult(clients *shared.ClientFactory, cmd *cobra.Command, reque
return nil
}

func printDatastoreBulkGetSuccess(cmd *cobra.Command, event *logger.LogEvent) {
func printDatastoreBulkGetSuccess(cmd *cobra.Command) {
}
13 changes: 5 additions & 8 deletions cmd/datastore/bulk_get_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (

"github.com/slackapi/slack-cli/internal/app"
"github.com/slackapi/slack-cli/internal/config"
"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/slackcontext"
Expand All @@ -36,11 +35,9 @@ type BulkGetPkgMock struct {
mock.Mock
}

func (m *BulkGetPkgMock) BulkGet(ctx context.Context, clients *shared.ClientFactory, log *logger.Logger, query types.AppDatastoreBulkGet) (*logger.LogEvent, error) {
m.Called(ctx, clients, log, query)
log.Data["bulkGetResult"] = types.AppDatastoreBulkGetResult{}
log.Log("info", "on_bulk_get_result")
return log.SuccessEvent(), nil
func (m *BulkGetPkgMock) BulkGet(ctx context.Context, clients *shared.ClientFactory, query types.AppDatastoreBulkGet) (types.AppDatastoreBulkGetResult, error) {
m.Called(ctx, clients, query)
return types.AppDatastoreBulkGetResult{}, nil
}

func TestBulkGetCommandPreRun(t *testing.T) {
Expand Down Expand Up @@ -185,7 +182,7 @@ func TestBulkGetCommand(t *testing.T) {
// Prepare mocked command
bulkGetMock := new(BulkGetPkgMock)
BulkGet = bulkGetMock.BulkGet
bulkGetMock.On("BulkGet", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
bulkGetMock.On("BulkGet", mock.Anything, mock.Anything, mock.Anything).Return(types.AppDatastoreBulkGetResult{}, nil)

cmd := NewBulkGetCommand(clients)
// TODO: could maybe refactor this to the os/fs mocks level to more clearly communicate "fake being in an app directory"
Expand All @@ -198,7 +195,7 @@ func TestBulkGetCommand(t *testing.T) {
// Perform test
err := cmd.ExecuteContext(ctx)
if assert.NoError(t, err) {
bulkGetMock.AssertCalled(t, "BulkGet", mock.Anything, mock.Anything, mock.Anything, tc.Query)
bulkGetMock.AssertCalled(t, "BulkGet", mock.Anything, mock.Anything, tc.Query)
}

// Cleanup when done
Expand Down
Loading
Loading