Skip to content
Open
38 changes: 38 additions & 0 deletions internal/icon/icon.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Copyright 2022-2026 Salesforce, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package icon

import (
"path/filepath"

"github.com/spf13/afero"
)

var SupportedExtensions = []string{".png", ".jpg", ".jpeg", ".gif"}
Comment thread
srtaalej marked this conversation as resolved.
Outdated

func ResolveIconPath(fs afero.Fs, manifestIcon string) string {
if manifestIcon != "" {
return manifestIcon
}
for _, dir := range []string{"assets", "."} {
for _, ext := range SupportedExtensions {
candidate := filepath.Join(dir, "icon"+ext)
if _, err := fs.Stat(candidate); err == nil {
return candidate
}
}
}
return ""
}
86 changes: 86 additions & 0 deletions internal/icon/icon_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
// Copyright 2022-2026 Salesforce, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package icon

import (
"testing"

"github.com/spf13/afero"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

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",
},
Comment on lines +31 to +34
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: Can we include a test that manifest icon is found before magic icons too?

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.

yes good catch!

"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"},
Comment thread
srtaalej marked this conversation as resolved.
Outdated
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)
})
}
}
18 changes: 4 additions & 14 deletions internal/pkg/apps/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@ package apps
import (
"context"
"fmt"
"os"
"strings"
"time"

"github.com/opentracing/opentracing-go"
"github.com/slackapi/slack-cli/internal/api"
"github.com/slackapi/slack-cli/internal/config"
"github.com/slackapi/slack-cli/internal/experiment"
"github.com/slackapi/slack-cli/internal/icon"
"github.com/slackapi/slack-cli/internal/pkg/manifest"
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/shared/types"
Expand Down Expand Up @@ -218,13 +218,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 := icon.ResolveIconPath(clients.Fs, slackManifest.Icon)
if iconPath != "" {
err = updateIcon(ctx, clients, iconPath, app.AppID, token, manifest.IsFunctionRuntimeSlackHosted())
if err != nil {
Expand Down Expand Up @@ -524,12 +519,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 := icon.ResolveIconPath(clients.Fs, slackManifest.Icon)
if iconPath != "" {
_, iconErr := clients.API().IconSet(ctx, clients.Fs, token, app.AppID, iconPath)
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: Unrelated to this PR but it's confusing to find the direct API call in this method but updateIcon called in the adjacent "install" method.

if iconErr != nil {
Expand Down
30 changes: 14 additions & 16 deletions internal/shared/types/slack_yaml.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ import (
"os"
"path/filepath"

"github.com/slackapi/slack-cli/internal/icon"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/spf13/afero"
)

type SlackYaml struct {
Expand All @@ -28,28 +30,24 @@ type SlackYaml struct {
}

// 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
}
} else {
if _, err := os.Stat(filepath.Join(wd, sy.Icon)); os.IsNotExist(err) {
return false
}
func (sy *SlackYaml) hasValidIconPath(fs afero.Fs) bool {
Comment thread
srtaalej marked this conversation as resolved.
Outdated
wd, err := os.Getwd()
if err != nil {
return true
}
if sy.Icon == "" {
sy.Icon = icon.ResolveIconPath(afero.NewBasePathFs(fs, wd), "")
} else {
if _, err := fs.Stat(filepath.Join(wd, sy.Icon)); os.IsNotExist(err) {
return false
}
}

return true
}

// Verify checks that the app manifest meets some basic requirements
func (sy *SlackYaml) Verify() error {
if !sy.hasValidIconPath() {
func (sy *SlackYaml) Verify(fs afero.Fs) error {
Comment thread
srtaalej marked this conversation as resolved.
Outdated
if !sy.hasValidIconPath(fs) {
return slackerror.New("Please specify a valid icon path in app manifest")
}
return nil
Expand Down
32 changes: 29 additions & 3 deletions internal/shared/types/slack_yaml_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"path/filepath"
"testing"

"github.com/spf13/afero"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -50,6 +51,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) {},
Expand All @@ -67,7 +93,7 @@ func Test_SlackYaml_hasValidIconPath(t *testing.T) {
defer func() { require.NoError(t, os.Chdir(origDir)) }()

sy := &SlackYaml{Icon: tc.icon}
assert.Equal(t, tc.expected, sy.hasValidIconPath())
assert.Equal(t, tc.expected, sy.hasValidIconPath(afero.NewOsFs()))
})
}
}
Expand Down Expand Up @@ -103,9 +129,9 @@ func Test_SlackYaml_Verify(t *testing.T) {

sy := &SlackYaml{Icon: tc.icon}
if tc.expectErr {
assert.Error(t, sy.Verify())
assert.Error(t, sy.Verify(afero.NewOsFs()))
} else {
assert.NoError(t, sy.Verify())
assert.NoError(t, sy.Verify(afero.NewOsFs()))
}
})
}
Expand Down
Loading