Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
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
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
// Environment Variable constants
const slackAccessibleEnv = "ACCESSIBLE"
const slackAutoRequestAAAEnv = "SLACK_AUTO_REQUEST_AAA"
const slackCLIAppIconPathEnv = "SLACK_CLI_APP_ICON_PATH"
const slackConfigDirEnv = "SLACK_CONFIG_DIR"
const slackDisableTelemetryEnv = "SLACK_DISABLE_TELEMETRY"
const slackTestTraceEnv = "SLACK_TEST_TRACE"
Expand All @@ -43,6 +44,7 @@ type Config struct {
APIHostFlag string
APIHostResolved string
AppFlag string
AppIconPathFlag string
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.

👾 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

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
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,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
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
5 changes: 1 addition & 4 deletions internal/icon/icon.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,7 @@ import (
"github.com/spf13/afero"
)

func ResolveIconPath(fs afero.Fs, manifestIcon string) string {
if manifestIcon != "" {
return manifestIcon
}
func ResolveIconPath(fs afero.Fs) string {
supportedExtensions := []string{".png", ".jpg", ".jpeg", ".gif"}
for _, dir := range []string{"assets", "."} {
for _, ext := range supportedExtensions {
Expand Down
16 changes: 3 additions & 13 deletions internal/icon/icon_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,9 @@ import (

func Test_ResolveIconPath(t *testing.T) {
tests := map[string]struct {
manifestIcon string
files []string
expected string
files []string
expected string
}{
"manifest icon set returns it directly": {
manifestIcon: "custom/my-icon.png",
expected: "custom/my-icon.png",
},
"manifest icon preferred over magic icon files": {
manifestIcon: "custom/my-icon.png",
files: []string{"assets/icon.png", "icon.png"},
expected: "custom/my-icon.png",
},
"assets/icon.png found": {
files: []string{"assets/icon.png"},
expected: "assets/icon.png",
Expand Down Expand Up @@ -84,7 +74,7 @@ func Test_ResolveIconPath(t *testing.T) {
for _, f := range tc.files {
require.NoError(t, afero.WriteFile(fs, f, []byte("img"), 0o644))
}
result := ResolveIconPath(fs, tc.manifestIcon)
result := ResolveIconPath(fs)
assert.Equal(t, tc.expected, result)
})
}
Expand Down
27 changes: 24 additions & 3 deletions internal/pkg/apps/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,7 @@ func Install(ctx context.Context, clients *shared.ClientFactory, auth types.Slac
}
}

// upload icon, default to assets/icon.{png,jpg,jpeg,gif} or icon.{png,jpg,jpeg,gif}
iconPath := icon.ResolveIconPath(clients.Fs, slackManifest.Icon)
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 @@ -519,7 +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) {
iconPath := icon.ResolveIconPath(clients.Fs, slackManifest.Icon)
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 @@ -639,6 +638,28 @@ 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 > assets/ and root fallback
func resolveIconPath(ctx context.Context, clients *shared.ClientFactory, manifestIcon string) string {
if envIconPath := clients.Config.AppIconPathFlag; envIconPath != "" {
if _, err := clients.Fs.Stat(envIconPath); err == nil {
return envIconPath
}
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

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 := clients.Fs.Stat(manifestIcon); err == nil {
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 ""
}
return icon.ResolveIconPath(clients.Fs)
}

// 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
81 changes: 81 additions & 0 deletions internal/pkg/apps/install_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"github.com/slackapi/slack-cli/internal/shared/types"
"github.com/slackapi/slack-cli/internal/slackcontext"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/spf13/afero"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -1728,6 +1729,86 @@ func TestContinueDespiteWarning(t *testing.T) {
}
}

func Test_resolveIconPath(t *testing.T) {
tests := map[string]struct {
envIconPath string
manifestIcon string
setupFiles func(t *testing.T, fs afero.Fs)
expected string
}{
"env var takes priority over manifest icon": {
envIconPath: "env-icon.png",
manifestIcon: "manifest-icon.png",
setupFiles: func(t *testing.T, fs afero.Fs) {
require.NoError(t, afero.WriteFile(fs, "env-icon.png", []byte("img"), 0o644))
require.NoError(t, afero.WriteFile(fs, "manifest-icon.png", []byte("img"), 0o644))
},
expected: "env-icon.png",
},
"env var takes priority over fallback": {
envIconPath: "env-icon.png",
setupFiles: func(t *testing.T, fs afero.Fs) {
require.NoError(t, afero.WriteFile(fs, "env-icon.png", []byte("img"), 0o644))
require.NoError(t, afero.WriteFile(fs, "assets/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, fs afero.Fs) {
require.NoError(t, afero.WriteFile(fs, "manifest-icon.png", []byte("img"), 0o644))
},
expected: "manifest-icon.png",
},
"falls back to assets/icon.png when no env var or manifest": {
setupFiles: func(t *testing.T, fs afero.Fs) {
require.NoError(t, afero.WriteFile(fs, "assets/icon.png", []byte("img"), 0o644))
},
expected: "assets/icon.png",
},
"falls back to icon.png in root when no assets": {
setupFiles: func(t *testing.T, fs afero.Fs) {
require.NoError(t, afero.WriteFile(fs, "icon.png", []byte("img"), 0o644))
},
expected: "icon.png",
},
"returns empty when no icon found": {
setupFiles: func(t *testing.T, fs afero.Fs) {},
expected: "",
},
"env var file not found returns empty": {
envIconPath: "missing-icon.png",
manifestIcon: "manifest-icon.png",
setupFiles: func(t *testing.T, fs afero.Fs) {
require.NoError(t, afero.WriteFile(fs, "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, fs afero.Fs) {
require.NoError(t, afero.WriteFile(fs, "assets/icon.png", []byte("img"), 0o644))
},
expected: "",
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
clientsMock := shared.NewClientsMock()
clientsMock.AddDefaultMocks()
clientsMock.Config.AppIconPathFlag = tc.envIconPath
tc.setupFiles(t, clientsMock.Fs)
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