From e7676db6c5cdb54a4581b3d581bb98581929f433 Mon Sep 17 00:00:00 2001 From: Ale Mercado Date: Mon, 20 Apr 2026 12:01:13 -0400 Subject: [PATCH 1/3] fix: route icon upload by app type in updateIcon --- internal/pkg/apps/install.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/internal/pkg/apps/install.go b/internal/pkg/apps/install.go index 1e3ee7df..9309bc39 100644 --- a/internal/pkg/apps/install.go +++ b/internal/pkg/apps/install.go @@ -226,7 +226,7 @@ func Install(ctx context.Context, clients *shared.ClientFactory, auth types.Slac } } if iconPath != "" { - err = updateIcon(ctx, clients, iconPath, app.AppID, token) + err = updateIcon(ctx, clients, iconPath, app.AppID, token, manifest.IsFunctionRuntimeSlackHosted()) if err != nil { clients.IO.PrintDebug(ctx, "icon error: %s", err) _, _ = clients.IO.WriteOut().Write([]byte(style.SectionSecondaryf("Error updating app icon: %s", err))) @@ -651,7 +651,7 @@ func appendLocalToDisplayName(manifest *types.AppManifest) { } // updateIcon will upload the new icon to the Slack API -func updateIcon(ctx context.Context, clients *shared.ClientFactory, iconPath, appID string, token string) error { +func updateIcon(ctx context.Context, clients *shared.ClientFactory, iconPath, appID string, token string, isHosted bool) error { var span opentracing.Span span, ctx = opentracing.StartSpanFromContext(ctx, "updateIcon") defer span.Finish() @@ -659,10 +659,12 @@ func updateIcon(ctx context.Context, clients *shared.ClientFactory, iconPath, ap clients.IO.PrintDebug(ctx, "uploading icon") var err error - if clients.Config.WithExperimentOn(experiment.SetIcon) { + if isHosted { + _, err = clients.API().Icon(ctx, clients.Fs, token, appID, iconPath) + } else if clients.Config.WithExperimentOn(experiment.SetIcon) { _, err = clients.API().IconSet(ctx, clients.Fs, token, appID, iconPath) } else { - _, err = clients.API().Icon(ctx, clients.Fs, token, appID, iconPath) + return nil } if err != nil { // TODO: separate the icon upload into a different function because if an error is returned From 1527e03a5daadefcf1a4e5ea13a6c82e71e7d855 Mon Sep 17 00:00:00 2001 From: Ale Mercado Date: Tue, 21 Apr 2026 11:55:23 -0400 Subject: [PATCH 2/3] default to apps.icon.set --- internal/pkg/apps/install.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/pkg/apps/install.go b/internal/pkg/apps/install.go index bcb956fb..f5089994 100644 --- a/internal/pkg/apps/install.go +++ b/internal/pkg/apps/install.go @@ -656,10 +656,10 @@ func updateIcon(ctx context.Context, clients *shared.ClientFactory, iconPath, ap defer span.Finish() var err error - if isHosted { - _, err = clients.API().Icon(ctx, clients.Fs, token, appID, iconPath) - } else if clients.Config.WithExperimentOn(experiment.SetIcon) { + if clients.Config.WithExperimentOn(experiment.SetIcon) { _, err = clients.API().IconSet(ctx, clients.Fs, token, appID, iconPath) + } else if isHosted { + _, err = clients.API().Icon(ctx, clients.Fs, token, appID, iconPath) } else { return nil } From 65c85d868820834558d816edcb31d27f9284fa80 Mon Sep 17 00:00:00 2001 From: Ale Mercado Date: Tue, 28 Apr 2026 15:37:21 -0400 Subject: [PATCH 3/3] fix: add deprecation comment and unit tests for updateIcon routing Co-Authored-By: Claude --- internal/pkg/apps/install.go | 1 + internal/pkg/apps/install_test.go | 87 +++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+) diff --git a/internal/pkg/apps/install.go b/internal/pkg/apps/install.go index f5089994..81d5524e 100644 --- a/internal/pkg/apps/install.go +++ b/internal/pkg/apps/install.go @@ -659,6 +659,7 @@ func updateIcon(ctx context.Context, clients *shared.ClientFactory, iconPath, ap if clients.Config.WithExperimentOn(experiment.SetIcon) { _, err = clients.API().IconSet(ctx, clients.Fs, token, appID, iconPath) } else if isHosted { + // DEPRECATED: Prefer IconSet once the SetIcon experiment concludes _, err = clients.API().Icon(ctx, clients.Fs, token, appID, iconPath) } else { return nil diff --git a/internal/pkg/apps/install_test.go b/internal/pkg/apps/install_test.go index 2c44dc94..f9610888 100644 --- a/internal/pkg/apps/install_test.go +++ b/internal/pkg/apps/install_test.go @@ -16,12 +16,15 @@ package apps import ( "bytes" + "context" + "fmt" "testing" "github.com/slackapi/slack-cli/internal/api" "github.com/slackapi/slack-cli/internal/app" "github.com/slackapi/slack-cli/internal/cache" "github.com/slackapi/slack-cli/internal/config" + "github.com/slackapi/slack-cli/internal/experiment" "github.com/slackapi/slack-cli/internal/shared" "github.com/slackapi/slack-cli/internal/shared/types" "github.com/slackapi/slack-cli/internal/slackcontext" @@ -1724,3 +1727,87 @@ func TestContinueDespiteWarning(t *testing.T) { }) } } + +func Test_updateIcon(t *testing.T) { + tests := map[string]struct { + isHosted bool + experimentOn bool + expectIconSet bool + expectIcon bool + expectSkip bool + mockError error + expectedError bool + }{ + "experiment on + hosted app uses IconSet": { + isHosted: true, + experimentOn: true, + expectIconSet: true, + }, + "experiment on + non-hosted app uses IconSet": { + isHosted: false, + experimentOn: true, + expectIconSet: true, + }, + "experiment off + hosted app uses Icon": { + isHosted: true, + experimentOn: false, + expectIcon: true, + }, + "experiment off + non-hosted app skips upload": { + isHosted: false, + experimentOn: false, + expectSkip: true, + }, + "returns error from IconSet": { + isHosted: false, + experimentOn: true, + expectIconSet: true, + mockError: fmt.Errorf("api error"), + expectedError: true, + }, + "returns error from Icon": { + isHosted: true, + experimentOn: false, + expectIcon: true, + mockError: fmt.Errorf("api error"), + expectedError: true, + }, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + ctx := slackcontext.MockContext(t.Context()) + clientsMock := shared.NewClientsMock() + clientsMock.AddDefaultMocks() + + if tc.experimentOn { + clientsMock.Config.ExperimentsFlag = []string{string(experiment.SetIcon)} + clientsMock.Config.LoadExperiments(ctx, func(_ context.Context, _ string, _ ...interface{}) {}) + } + + clientsMock.API.On("IconSet", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(api.IconResult{}, tc.mockError) + clientsMock.API.On("Icon", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(api.IconResult{}, tc.mockError) + + clients := shared.NewClientFactory(clientsMock.MockClientFactory()) + err := updateIcon(ctx, clients, "icon.png", "A001", "xoxe-token", tc.isHosted) + + if tc.expectedError { + require.Error(t, err) + } else { + require.NoError(t, err) + } + + if tc.expectIconSet { + clientsMock.API.AssertCalled(t, "IconSet", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + clientsMock.API.AssertNotCalled(t, "Icon") + } else if tc.expectIcon { + clientsMock.API.AssertCalled(t, "Icon", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + clientsMock.API.AssertNotCalled(t, "IconSet") + } else if tc.expectSkip { + clientsMock.API.AssertNotCalled(t, "Icon") + clientsMock.API.AssertNotCalled(t, "IconSet") + } + }) + } +}