Skip to content

feat: support SLACK_CLI_APP_ICON_PATH env var for icon override #519

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

feat: support SLACK_CLI_APP_ICON_PATH env var for icon override #519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b626cec
feat: support SLACK_CLI_APP_ICON_PATH env var for icon override
srtaalej May 1, 2026
95ded37
merge: resolve conflict with main in install_test.go
srtaalej May 1, 2026
d7e7ccd
Update internal/config/config.go
srtaalej May 4, 2026
d599052
fix: address review feedback for icon path resolution
srtaalej May 4, 2026
87a4646
Update internal/pkg/apps/install.go
srtaalej May 5, 2026
061e54f
fix: update test to match manifest icon early-return behavior
srtaalej May 5, 2026
2778c01
Merge branch 'main' into ale-icon-env-var
srtaalej May 5, 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
2 changes: 2 additions & 0 deletions internal/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
)

// Environment Variable constants
const slackCLIAppIconPathEnv = "SLACK_CLI_APP_ICON_PATH"
const slackAutoRequestAAAEnv = "SLACK_AUTO_REQUEST_AAA"
const slackConfigDirEnv = "SLACK_CONFIG_DIR"
const slackDisableTelemetryEnv = "SLACK_DISABLE_TELEMETRY"
Expand All @@ -41,6 +42,7 @@ type Config struct {
APIHostFlag string
APIHostResolved string
AppFlag string
AppIconPathFlag string
Copy link
Copy Markdown
Member

@zimeg zimeg May 4, 2026

Choose a reason for hiding this comment

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

👾 thought: Interesting to find "app icon path" reads more clear than "CLI app icon path" but this ought not block decision on:

- SLACK_APP_ICON_PATH
+ SLACK_CLI_APP_ICON_PATH

🌲 ramble: I'm curious now again if "file" path needs to be specified in these values? Curious if path is clear enough on it's own...

Copy link
Copy Markdown
Contributor Author

@srtaalej srtaalej May 5, 2026

Choose a reason for hiding this comment

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

i can change it to CLIAppIconPath! i was matching it to the other vars here but it doesnt have to

AutoRequestAAAFlag bool
ConfigDirFlag string
DebugEnabled bool
Expand Down
6 changes: 6 additions & 0 deletions internal/config/dotenv.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ func (c *Config) LoadEnvironmentVariables() error {
c.ConfigDirFlag = configDir
}

// Load app icon path from environment variables
var appIconPath = strings.TrimSpace(c.os.Getenv(slackCLIAppIconPathEnv))
if appIconPath != "" {
c.AppIconPathFlag = appIconPath
}

// Disable telemetry if either disable-telemetry or test-version environment variables
var disableTelemetry = strings.TrimSpace(c.os.Getenv(slackDisableTelemetryEnv))
var testVersion = strings.TrimSpace(c.os.Getenv(version.EnvTestVersion))
Expand Down
21 changes: 21 additions & 0 deletions internal/config/dotenv_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,27 @@ func Test_DotEnv_LoadEnvironmentVariables(t *testing.T) {
assert.Equal(t, false, cfg.AutoRequestAAAFlag)
},
},
"SLACK_CLI_APP_ICON_PATH=/path/to/icon.png should set AppIconPathFlag": {
envName: "SLACK_CLI_APP_ICON_PATH",
envValue: "/path/to/icon.png",
assertOnConfig: func(t *testing.T, cfg *Config) {
assert.Equal(t, "/path/to/icon.png", cfg.AppIconPathFlag)
},
},
"SLACK_CLI_APP_ICON_PATH= should not set AppIconPathFlag": {
envName: "SLACK_CLI_APP_ICON_PATH",
envValue: "",
assertOnConfig: func(t *testing.T, cfg *Config) {
assert.Equal(t, "", cfg.AppIconPathFlag)
},
},
"SLACK_CLI_APP_ICON_PATH with whitespace should be trimmed": {
envName: "SLACK_CLI_APP_ICON_PATH",
envValue: " /path/to/icon.png ",
assertOnConfig: func(t *testing.T, cfg *Config) {
assert.Equal(t, "/path/to/icon.png", cfg.AppIconPathFlag)
},
},
"SLACK_CONFIG_DIR=/path/to/config should set the config dir": {
envName: "SLACK_CONFIG_DIR",
envValue: "/path/to/config",
Expand Down
40 changes: 27 additions & 13 deletions internal/pkg/apps/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,13 +218,7 @@ func Install(ctx context.Context, clients *shared.ClientFactory, auth types.Slac
}
}

// upload icon, default to icon.png
var iconPath = slackManifest.Icon
if iconPath == "" {
if _, err := os.Stat("icon.png"); !os.IsNotExist(err) {
iconPath = "icon.png"
}
}
Comment on lines -221 to -227
Copy link
Copy Markdown
Member

@zimeg zimeg May 4, 2026

Choose a reason for hiding this comment

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

📸 thought: Keeping this inline might be alright to avoided redirected logic?

⛈️ ramble: I might suggest we instead also extract the "upload" logic that follows this since it's duplicated too, but I'd much prefer the Install and InstallLocalApp functions be merged because the logic is almost identical if this is changing-

iconPath := resolveIconPath(ctx, clients, slackManifest.Icon)
if iconPath != "" {
err = updateIcon(ctx, clients, iconPath, app.AppID, token, manifest.IsFunctionRuntimeSlackHosted())
if err != nil {
Expand Down Expand Up @@ -524,12 +518,7 @@ func InstallLocalApp(ctx context.Context, clients *shared.ClientFactory, orgGran

// upload icon for non-hosted apps (gated behind set-icon experiment)
if clients.Config.WithExperimentOn(experiment.SetIcon) {
var iconPath = slackManifest.Icon
if iconPath == "" {
if _, err := os.Stat("icon.png"); !os.IsNotExist(err) {
iconPath = "icon.png"
}
}
iconPath := resolveIconPath(ctx, clients, slackManifest.Icon)
if iconPath != "" {
_, iconErr := clients.API().IconSet(ctx, clients.Fs, token, app.AppID, iconPath)
if iconErr != nil {
Expand Down Expand Up @@ -649,6 +638,31 @@ func appendLocalToDisplayName(manifest *types.AppManifest) {
}
}

// resolveIconPath determines the icon file path using the priority chain:
// SLACK_CLI_APP_ICON_PATH env var > manifest icon field > icon.png in project root
func resolveIconPath(ctx context.Context, clients *shared.ClientFactory, manifestIcon string) string {
if envIconPath := clients.Config.AppIconPathFlag; envIconPath != "" {
if _, err := os.Stat(envIconPath); !os.IsNotExist(err) {
return envIconPath
}
Comment on lines +645 to +647
Copy link
Copy Markdown
Member

@zimeg zimeg May 4, 2026

Choose a reason for hiding this comment

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

🪬 question: Is this a check we should also include for manifestIcon values?

clients.IO.PrintDebug(ctx, "SLACK_CLI_APP_ICON_PATH file not found: %s", envIconPath)
_, _ = clients.IO.WriteOut().Write([]byte(style.SectionSecondaryf("Warning: icon path from SLACK_CLI_APP_ICON_PATH not found: %s", envIconPath)))
Copy link
Copy Markdown
Member

@zimeg zimeg May 4, 2026

Choose a reason for hiding this comment

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

⚠️ issue: Falling through might be unexpected here if the SLACK_CLI_APP_ICON_PATH is set. IMHO we should print the warning and return no value instead?

return ""
}
if manifestIcon != "" {
if _, err := os.Stat(manifestIcon); !os.IsNotExist(err) {
return manifestIcon
}
Comment thread
srtaalej marked this conversation as resolved.
clients.IO.PrintDebug(ctx, "manifest icon file not found: %s", manifestIcon)
_, _ = clients.IO.WriteOut().Write([]byte(style.SectionSecondaryf("Warning: icon path from manifest not found: %s", manifestIcon)))
return ""
}
if _, err := os.Stat("icon.png"); !os.IsNotExist(err) {
return "icon.png"
}
return ""
}

// 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 {
var span opentracing.Span
Expand Down
88 changes: 88 additions & 0 deletions internal/pkg/apps/install_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import (
"bytes"
"context"
"fmt"
"os"
"path/filepath"
"testing"

"github.com/slackapi/slack-cli/internal/api"
Expand Down Expand Up @@ -1728,6 +1730,92 @@ func TestContinueDespiteWarning(t *testing.T) {
}
}

func Test_resolveIconPath(t *testing.T) {
tests := map[string]struct {
envIconPath string
manifestIcon string
setupFiles func(t *testing.T, dir string)
expected string
}{
"env var takes priority over manifest icon": {
envIconPath: "env-icon.png",
manifestIcon: "manifest-icon.png",
setupFiles: func(t *testing.T, dir string) {
require.NoError(t, os.WriteFile(filepath.Join(dir, "env-icon.png"), []byte("img"), 0o644))
require.NoError(t, os.WriteFile(filepath.Join(dir, "manifest-icon.png"), []byte("img"), 0o644))
},
expected: "env-icon.png",
},
"env var takes priority over icon.png fallback": {
envIconPath: "env-icon.png",
setupFiles: func(t *testing.T, dir string) {
require.NoError(t, os.WriteFile(filepath.Join(dir, "env-icon.png"), []byte("img"), 0o644))
require.NoError(t, os.WriteFile(filepath.Join(dir, "icon.png"), []byte("img"), 0o644))
},
expected: "env-icon.png",
},
"manifest icon used when no env var": {
manifestIcon: "manifest-icon.png",
setupFiles: func(t *testing.T, dir string) {
require.NoError(t, os.WriteFile(filepath.Join(dir, "manifest-icon.png"), []byte("img"), 0o644))
},
expected: "manifest-icon.png",
},
"falls back to icon.png when no env var or manifest": {
setupFiles: func(t *testing.T, dir string) {
require.NoError(t, os.WriteFile(filepath.Join(dir, "icon.png"), []byte("img"), 0o644))
},
expected: "icon.png",
},
"returns empty when no icon found": {
setupFiles: func(t *testing.T, dir string) {},
expected: "",
},
"env var file not found returns empty": {
envIconPath: "missing-icon.png",
manifestIcon: "manifest-icon.png",
setupFiles: func(t *testing.T, dir string) {
require.NoError(t, os.WriteFile(filepath.Join(dir, "manifest-icon.png"), []byte("img"), 0o644))
},
expected: "",
},
"manifest icon file not found returns empty with warning": {
manifestIcon: "missing-manifest-icon.png",
setupFiles: func(t *testing.T, dir string) {
require.NoError(t, os.WriteFile(filepath.Join(dir, "icon.png"), []byte("img"), 0o644))
},
expected: "",
},
"manifest icon file not found and no icon.png returns empty": {
manifestIcon: "missing-manifest-icon.png",
setupFiles: func(t *testing.T, dir string) {},
expected: "",
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
dir := t.TempDir()
tc.setupFiles(t, dir)

origDir, err := os.Getwd()
require.NoError(t, err)
require.NoError(t, os.Chdir(dir))
defer func() { require.NoError(t, os.Chdir(origDir)) }()

ctx := slackcontext.MockContext(t.Context())
clientsMock := shared.NewClientsMock()
clientsMock.AddDefaultMocks()
clientsMock.Config.AppIconPathFlag = tc.envIconPath
output := &bytes.Buffer{}
clientsMock.IO.Stdout.SetOutput(output)
clients := shared.NewClientFactory(clientsMock.MockClientFactory())

result := resolveIconPath(ctx, clients, tc.manifestIcon)
assert.Equal(t, tc.expected, result)
})
}
}

func Test_updateIcon(t *testing.T) {
tests := map[string]struct {
isHosted bool
Expand Down
Loading