diff --git a/internal/pkg/apps/install.go b/internal/pkg/apps/install.go index 9dcd68ee..b89ce904 100644 --- a/internal/pkg/apps/install.go +++ b/internal/pkg/apps/install.go @@ -17,7 +17,7 @@ package apps import ( "context" "fmt" - "os" + "path/filepath" "strings" "time" @@ -30,6 +30,7 @@ import ( "github.com/slackapi/slack-cli/internal/shared/types" "github.com/slackapi/slack-cli/internal/slackerror" "github.com/slackapi/slack-cli/internal/style" + "github.com/spf13/afero" ) // Constants for onlyCreateUpdateAppManifest parameter @@ -40,6 +41,23 @@ const ( const additionalManifestInfoNotice = "App manifest contains some components that may require additional information" +var supportedIconExtensions = []string{".png", ".jpg", ".jpeg", ".gif"} + +func resolveIconPath(fs afero.Fs, manifestIcon string) string { + if manifestIcon != "" { + return manifestIcon + } + for _, dir := range []string{"assets", "."} { + for _, ext := range supportedIconExtensions { + candidate := filepath.Join(dir, "icon"+ext) + if _, err := fs.Stat(candidate); err == nil { + return candidate + } + } + } + return "" +} + // Install installs the app to a team func Install(ctx context.Context, clients *shared.ClientFactory, auth types.SlackAuth, onlyCreateUpdateAppManifest bool, app types.App, orgGrantWorkspaceID string) (types.App, types.InstallState, error) { span, ctx := opentracing.StartSpanFromContext(ctx, "pkg.apps.install") @@ -218,13 +236,8 @@ 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" - } - } + // upload icon, default to assets/icon.{png,jpg,jpeg,gif} or icon.{png,jpg,jpeg,gif} + iconPath := resolveIconPath(clients.Fs, slackManifest.Icon) if iconPath != "" { err = updateIcon(ctx, clients, iconPath, app.AppID, token) if err != nil { @@ -524,12 +537,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(clients.Fs, slackManifest.Icon) if iconPath != "" { _, iconErr := clients.API().IconSet(ctx, clients.Fs, token, app.AppID, iconPath) if iconErr != nil { diff --git a/internal/pkg/apps/install_test.go b/internal/pkg/apps/install_test.go index 2c44dc94..2be011a4 100644 --- a/internal/pkg/apps/install_test.go +++ b/internal/pkg/apps/install_test.go @@ -26,6 +26,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" @@ -1724,3 +1725,66 @@ func TestContinueDespiteWarning(t *testing.T) { }) } } + +func Test_resolveIconPath(t *testing.T) { + tests := map[string]struct { + manifestIcon string + files []string + expected string + }{ + "manifest icon set returns it directly": { + manifestIcon: "custom/my-icon.png", + expected: "custom/my-icon.png", + }, + "assets/icon.png found": { + files: []string{"assets/icon.png"}, + expected: "assets/icon.png", + }, + "assets/icon.jpg found": { + files: []string{"assets/icon.jpg"}, + expected: "assets/icon.jpg", + }, + "assets/icon.jpeg found": { + files: []string{"assets/icon.jpeg"}, + expected: "assets/icon.jpeg", + }, + "assets/icon.gif found": { + files: []string{"assets/icon.gif"}, + expected: "assets/icon.gif", + }, + "png wins over gif in assets": { + files: []string{"assets/icon.png", "assets/icon.gif"}, + expected: "assets/icon.png", + }, + "jpg wins over gif in assets": { + files: []string{"assets/icon.jpg", "assets/icon.gif"}, + expected: "assets/icon.jpg", + }, + "root icon.png found when no assets": { + files: []string{"icon.png"}, + expected: "icon.png", + }, + "root icon.jpg found when no assets": { + files: []string{"icon.jpg"}, + expected: "icon.jpg", + }, + "assets takes priority over root": { + files: []string{"assets/icon.gif", "icon.png"}, + expected: "assets/icon.gif", + }, + "no icon files returns empty": { + files: []string{}, + expected: "", + }, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + fs := afero.NewMemMapFs() + for _, f := range tc.files { + require.NoError(t, afero.WriteFile(fs, f, []byte("img"), 0o644)) + } + result := resolveIconPath(fs, tc.manifestIcon) + assert.Equal(t, tc.expected, result) + }) + } +} diff --git a/internal/shared/types/slack_yaml.go b/internal/shared/types/slack_yaml.go index 106135b2..1f8fea08 100644 --- a/internal/shared/types/slack_yaml.go +++ b/internal/shared/types/slack_yaml.go @@ -27,15 +27,19 @@ type SlackYaml struct { Hash string } +var supportedIconExtensions = []string{".png", ".jpg", ".jpeg", ".gif"} + // hasValidIconPath returns false if icon path is provided but is not valid and true otherwise func (sy *SlackYaml) hasValidIconPath() bool { - // verify icon path is valid if exists var wd, err = os.Getwd() if err == nil { - if sy.Icon == "" { // icon was not provided. Let's check if the default one exists - var defaultIconPath = "assets/icon.png" - if _, err := os.Stat(filepath.Join(wd, defaultIconPath)); !os.IsNotExist(err) { - sy.Icon = defaultIconPath + if sy.Icon == "" { + for _, ext := range supportedIconExtensions { + candidate := filepath.Join(wd, "assets", "icon"+ext) + if _, err := os.Stat(candidate); !os.IsNotExist(err) { + sy.Icon = filepath.Join("assets", "icon"+ext) + break + } } } else { if _, err := os.Stat(filepath.Join(wd, sy.Icon)); os.IsNotExist(err) { diff --git a/internal/shared/types/slack_yaml_test.go b/internal/shared/types/slack_yaml_test.go index f9264288..7276a284 100644 --- a/internal/shared/types/slack_yaml_test.go +++ b/internal/shared/types/slack_yaml_test.go @@ -50,6 +50,31 @@ func Test_SlackYaml_hasValidIconPath(t *testing.T) { }, expected: true, }, + "no icon with default assets/icon.jpg present returns true": { + icon: "", + setup: func(t *testing.T, dir string) { + require.NoError(t, os.MkdirAll(filepath.Join(dir, "assets"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "assets", "icon.jpg"), []byte("img"), 0o644)) + }, + expected: true, + }, + "no icon with default assets/icon.gif present returns true": { + icon: "", + setup: func(t *testing.T, dir string) { + require.NoError(t, os.MkdirAll(filepath.Join(dir, "assets"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "assets", "icon.gif"), []byte("img"), 0o644)) + }, + expected: true, + }, + "png takes priority over jpg in assets": { + icon: "", + setup: func(t *testing.T, dir string) { + require.NoError(t, os.MkdirAll(filepath.Join(dir, "assets"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "assets", "icon.png"), []byte("img"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "assets", "icon.jpg"), []byte("img"), 0o644)) + }, + expected: true, + }, "no icon and no default returns true": { icon: "", setup: func(t *testing.T, dir string) {},