Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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 @@ -22,6 +22,7 @@ import (
)

// Environment Variable constants
const slackAppIconPathEnv = "SLACK_CLI_APP_ICON_PATH"
Comment thread
srtaalej marked this conversation as resolved.
Outdated
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

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 @@ -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(slackAppIconPathEnv))
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: This might not capture values from the .env file also. I'm wondering if this extends to other values in this file so perhaps not a blocker?

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.

yup all other env vars are read from system !

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
34 changes: 21 additions & 13 deletions internal/pkg/apps/install.go
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"
}
}
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: 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,25 @@ 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
}
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.

🪬 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

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?

}
if manifestIcon != "" {
return manifestIcon
}
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
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 falls back to manifest": {
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.png",
},
"env var file not found falls back to icon.png": {
envIconPath: "missing-icon.png",
setupFiles: func(t *testing.T, dir string) {
require.NoError(t, os.WriteFile(filepath.Join(dir, "icon.png"), []byte("img"), 0o644))
},
expected: "icon.png",
},
"env var file not found and no fallback returns empty": {
envIconPath: "missing-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