Skip to content

feat: conclude set-icon experiment #623

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
srtaalej wants to merge 1 commit into
base: main
Choose a base branch
from
+15 −86
Open

feat: conclude set-icon experiment #623

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
2 changes: 1 addition & 1 deletion docs/reference/experiments.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ The following is a list of currently available experiments. We'll remove experim

- `lipgloss`: shows pretty styles.
- `manifest-sync`: resolves conflicting app manifest values.
- `set-icon`: enables icon upload for non-hosted apps ([PR#469](https://github.com/slackapi/slack-cli/pull/469)).

## Experiments changelog

Below is a list of updates related to experiments.

- **August 2026**: Concluded the `set-icon` experiment with full support for icon upload on all app types now enabled by default in the Slack CLI.

@lukegalbraithrussell lukegalbraithrussell Aug 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
- **August 2026**: Concluded the `set-icon` experiment with full support for icon upload on all app types now enabled by default in the Slack CLI.
- **August 2026**: Concluded the `set-icon` experiment; The Slack CLI now offers full support for icon upload on all app types by default.

just a little easier to read methinks

- **July 2026**: Added the `manifest-sync` experiment to resolve changed app manifest values between a project and app settings.
- **April 2026**: Concluded the `sandboxes` experiment with full support in the Slack CLI. Refer to the [`slack sandbox create`](/tools/slack-cli/reference/commands/slack_sandbox_create/), [`slack sandbox delete`](/tools/slack-cli/reference/commands/slack_sandbox_delete/), and [`slack sandbox list`](/tools/slack-cli/reference/commands/slack_sandbox_list/) commands for more details.
- **April 2026**: Added the `set-icon` experiment to enable icon upload for non-hosted apps.
Expand Down
4 changes: 0 additions & 4 deletions internal/experiment/experiment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,6 @@ const (

// Placeholder experiment is a placeholder for testing and does nothing... or does it?
Placeholder Experiment = "placeholder"

// SetIcon experiment enables icon upload for non-hosted apps.
SetIcon Experiment = "set-icon"
)

// AllExperiments is a list of all available experiments that can be enabled
Expand All @@ -49,7 +46,6 @@ var AllExperiments = []Experiment{
Lipgloss,
ManifestSync,
Placeholder,
SetIcon,
}

// EnabledExperiments is a list of experiments that are permanently enabled
Expand Down
37 changes: 11 additions & 26 deletions internal/pkg/apps/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ func Install(ctx context.Context, clients *shared.ClientFactory, auth types.Slac

iconPath := resolveIconPath(ctx, clients, slackManifest.Icon)
if iconPath != "" {
err = updateIcon(ctx, clients, iconPath, app.AppID, token, manifest.IsFunctionRuntimeSlackHosted())
err = updateIcon(ctx, clients, iconPath, app.AppID, token)
if err != nil {
clients.IO.PrintDebug(ctx, "icon error: %s", err)
_, _ = clients.IO.WriteOut().Write([]byte(style.SectionSecondaryf("Error updating app icon: %s", err)))
Expand Down Expand Up @@ -516,17 +516,14 @@ func InstallLocalApp(ctx context.Context, clients *shared.ClientFactory, orgGran
return app, result, installState, err
}

// upload icon for non-hosted apps (gated behind set-icon experiment)
if clients.Config.WithExperimentOn(experiment.SetIcon) {
iconPath := resolveIconPath(ctx, clients, slackManifest.Icon)
if iconPath != "" {
_, iconErr := clients.API().IconSet(ctx, clients.Fs, token, app.AppID, iconPath)
if iconErr != nil {
clients.IO.PrintDebug(ctx, "icon error: %s", iconErr)
_, _ = clients.IO.WriteOut().Write([]byte(style.SectionSecondaryf("Error updating app icon: %s", iconErr)))
} else {
_, _ = clients.IO.WriteOut().Write([]byte(style.SectionSecondaryf("Updated app icon: %s", iconPath)))
}
iconPath := resolveIconPath(ctx, clients, slackManifest.Icon)
if iconPath != "" {
_, iconErr := clients.API().IconSet(ctx, clients.Fs, token, app.AppID, iconPath)
if iconErr != nil {
clients.IO.PrintDebug(ctx, "icon error: %s", iconErr)
_, _ = clients.IO.WriteOut().Write([]byte(style.SectionSecondaryf("Error updating app icon: %s", iconErr)))
} else {
_, _ = clients.IO.WriteOut().Write([]byte(style.SectionSecondaryf("Updated app icon: %s", iconPath)))
}
}

Expand Down Expand Up @@ -661,28 +658,16 @@ func resolveIconPath(ctx context.Context, clients *shared.ClientFactory, manifes
}

// updateIcon will upload the new icon to the Slack API
func updateIcon(ctx context.Context, clients *shared.ClientFactory, iconPath, appID string, token string, isHosted bool) error {
func updateIcon(ctx context.Context, clients *shared.ClientFactory, iconPath, appID string, token string) error {

@zimeg zimeg Aug 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🪓 suggestion: This function is used once in Install and I think we can now inline the API call to match InstallLocal patterns? I'd like to avoid separate patterns now that the experiment has concluded.

var span opentracing.Span
span, ctx = opentracing.StartSpanFromContext(ctx, "updateIcon")
defer span.Finish()

var err error
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
}
_, err := clients.API().IconSet(ctx, clients.Fs, token, appID, iconPath)
if err != nil {
// TODO: separate the icon upload into a different function because if an error is returned
// the new app_id might be ignored and next time we'll create another app.
return fmt.Errorf("%s %s", err, iconPath)
}

// Save a md5 hash of the icon in environments.yaml
// env.IconHash = iconResp.MD5Hash
return nil
}

Expand Down
58 changes: 3 additions & 55 deletions internal/pkg/apps/install_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,13 @@ 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"
Expand Down Expand Up @@ -1811,45 +1809,11 @@ func Test_resolveIconPath(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,
},
"succeeds with IconSet": {},

@zimeg zimeg Aug 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🧪 note: This test doesn't seem so useful without mock data included. Another comment suggested deleting the function it's testing and IMHO this is supported here. Otherwise I'd suggest including:

  • mockFilePath
  • mockSetIconResponse

"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,
},
Expand All @@ -1860,35 +1824,19 @@ func Test_updateIcon(t *testing.T) {
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)
err := updateIcon(ctx, clients, "icon.png", "A001", "xoxe-token")

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")
}
clientsMock.API.AssertCalled(t, "IconSet", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything)
})
}
}
Loading