Skip to content

Commit c9abb30

Browse files
srtaalejsrtaalej
committed
refactor: extract icon resolution to internal/icon package
Consolidate duplicated icon path resolution logic from install.go and slack_yaml.go into a shared internal/icon package per review feedback.
1 parent 5176129 commit c9abb30

6 files changed

Lines changed: 145 additions & 108 deletions

File tree

‎internal/icon/icon.go‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
// Copyright 2022-2026 Salesforce, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package icon
16+
17+
import (
18+
"path/filepath"
19+
20+
"github.com/spf13/afero"
21+
)
22+
23+
var SupportedExtensions = []string{".png", ".jpg", ".jpeg", ".gif"}
24+
25+
func ResolveIconPath(fs afero.Fs, manifestIcon string) string {
26+
if manifestIcon != "" {
27+
return manifestIcon
28+
}
29+
for _, dir := range []string{"assets", "."} {
30+
for _, ext := range SupportedExtensions {
31+
candidate := filepath.Join(dir, "icon"+ext)
32+
if _, err := fs.Stat(candidate); err == nil {
33+
return candidate
34+
}
35+
}
36+
}
37+
return ""
38+
}

‎internal/icon/icon_test.go‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
// Copyright 2022-2026 Salesforce, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package icon
16+
17+
import (
18+
"testing"
19+
20+
"github.com/spf13/afero"
21+
"github.com/stretchr/testify/assert"
22+
"github.com/stretchr/testify/require"
23+
)
24+
25+
func Test_ResolveIconPath(t *testing.T) {
26+
tests := map[string]struct {
27+
manifestIcon string
28+
files []string
29+
expected string
30+
}{
31+
"manifest icon set returns it directly": {
32+
manifestIcon: "custom/my-icon.png",
33+
expected: "custom/my-icon.png",
34+
},
35+
"assets/icon.png found": {
36+
files: []string{"assets/icon.png"},
37+
expected: "assets/icon.png",
38+
},
39+
"assets/icon.jpg found": {
40+
files: []string{"assets/icon.jpg"},
41+
expected: "assets/icon.jpg",
42+
},
43+
"assets/icon.jpeg found": {
44+
files: []string{"assets/icon.jpeg"},
45+
expected: "assets/icon.jpeg",
46+
},
47+
"assets/icon.gif found": {
48+
files: []string{"assets/icon.gif"},
49+
expected: "assets/icon.gif",
50+
},
51+
"png wins over gif in assets": {
52+
files: []string{"assets/icon.png", "assets/icon.gif"},
53+
expected: "assets/icon.png",
54+
},
55+
"jpg wins over gif in assets": {
56+
files: []string{"assets/icon.jpg", "assets/icon.gif"},
57+
expected: "assets/icon.jpg",
58+
},
59+
"root icon.png found when no assets": {
60+
files: []string{"icon.png"},
61+
expected: "icon.png",
62+
},
63+
"root icon.jpg found when no assets": {
64+
files: []string{"icon.jpg"},
65+
expected: "icon.jpg",
66+
},
67+
"assets takes priority over root": {
68+
files: []string{"assets/icon.gif", "icon.png"},
69+
expected: "assets/icon.gif",
70+
},
71+
"no icon files returns empty": {
72+
files: []string{},
73+
expected: "",
74+
},
75+
}
76+
for name, tc := range tests {
77+
t.Run(name, func(t *testing.T) {
78+
fs := afero.NewMemMapFs()
79+
for _, f := range tc.files {
80+
require.NoError(t, afero.WriteFile(fs, f, []byte("img"), 0o644))
81+
}
82+
result := ResolveIconPath(fs, tc.manifestIcon)
83+
assert.Equal(t, tc.expected, result)
84+
})
85+
}
86+
}

‎internal/pkg/apps/install.go‎

Lines changed: 3 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -17,20 +17,19 @@ package apps
1717
import (
1818
"context"
1919
"fmt"
20-
"path/filepath"
2120
"strings"
2221
"time"
2322

2423
"github.com/opentracing/opentracing-go"
2524
"github.com/slackapi/slack-cli/internal/api"
2625
"github.com/slackapi/slack-cli/internal/config"
2726
"github.com/slackapi/slack-cli/internal/experiment"
27+
"github.com/slackapi/slack-cli/internal/icon"
2828
"github.com/slackapi/slack-cli/internal/pkg/manifest"
2929
"github.com/slackapi/slack-cli/internal/shared"
3030
"github.com/slackapi/slack-cli/internal/shared/types"
3131
"github.com/slackapi/slack-cli/internal/slackerror"
3232
"github.com/slackapi/slack-cli/internal/style"
33-
"github.com/spf13/afero"
3433
)
3534

3635
// Constants for onlyCreateUpdateAppManifest parameter
@@ -41,23 +40,6 @@ const (
4140

4241
const additionalManifestInfoNotice = "App manifest contains some components that may require additional information"
4342

44-
var supportedIconExtensions = []string{".png", ".jpg", ".jpeg", ".gif"}
45-
46-
func resolveIconPath(fs afero.Fs, manifestIcon string) string {
47-
if manifestIcon != "" {
48-
return manifestIcon
49-
}
50-
for _, dir := range []string{"assets", "."} {
51-
for _, ext := range supportedIconExtensions {
52-
candidate := filepath.Join(dir, "icon"+ext)
53-
if _, err := fs.Stat(candidate); err == nil {
54-
return candidate
55-
}
56-
}
57-
}
58-
return ""
59-
}
60-
6143
// Install installs the app to a team
6244
func Install(ctx context.Context, clients *shared.ClientFactory, auth types.SlackAuth, onlyCreateUpdateAppManifest bool, app types.App, orgGrantWorkspaceID string) (types.App, types.InstallState, error) {
6345
span, ctx := opentracing.StartSpanFromContext(ctx, "pkg.apps.install")
@@ -237,7 +219,7 @@ func Install(ctx context.Context, clients *shared.ClientFactory, auth types.Slac
237219
}
238220

239221
// upload icon, default to assets/icon.{png,jpg,jpeg,gif} or icon.{png,jpg,jpeg,gif}
240-
iconPath := resolveIconPath(clients.Fs, slackManifest.Icon)
222+
iconPath := icon.ResolveIconPath(clients.Fs, slackManifest.Icon)
241223
if iconPath != "" {
242224
err = updateIcon(ctx, clients, iconPath, app.AppID, token)
243225
if err != nil {
@@ -537,7 +519,7 @@ func InstallLocalApp(ctx context.Context, clients *shared.ClientFactory, orgGran
537519

538520
// upload icon for non-hosted apps (gated behind set-icon experiment)
539521
if clients.Config.WithExperimentOn(experiment.SetIcon) {
540-
iconPath := resolveIconPath(clients.Fs, slackManifest.Icon)
522+
iconPath := icon.ResolveIconPath(clients.Fs, slackManifest.Icon)
541523
if iconPath != "" {
542524
_, iconErr := clients.API().IconSet(ctx, clients.Fs, token, app.AppID, iconPath)
543525
if iconErr != nil {

‎internal/pkg/apps/install_test.go‎

Lines changed: 0 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ import (
2626
"github.com/slackapi/slack-cli/internal/shared/types"
2727
"github.com/slackapi/slack-cli/internal/slackcontext"
2828
"github.com/slackapi/slack-cli/internal/slackerror"
29-
"github.com/spf13/afero"
3029
"github.com/stretchr/testify/assert"
3130
"github.com/stretchr/testify/mock"
3231
"github.com/stretchr/testify/require"
@@ -1725,66 +1724,3 @@ func TestContinueDespiteWarning(t *testing.T) {
17251724
})
17261725
}
17271726
}
1728-
1729-
func Test_resolveIconPath(t *testing.T) {
1730-
tests := map[string]struct {
1731-
manifestIcon string
1732-
files []string
1733-
expected string
1734-
}{
1735-
"manifest icon set returns it directly": {
1736-
manifestIcon: "custom/my-icon.png",
1737-
expected: "custom/my-icon.png",
1738-
},
1739-
"assets/icon.png found": {
1740-
files: []string{"assets/icon.png"},
1741-
expected: "assets/icon.png",
1742-
},
1743-
"assets/icon.jpg found": {
1744-
files: []string{"assets/icon.jpg"},
1745-
expected: "assets/icon.jpg",
1746-
},
1747-
"assets/icon.jpeg found": {
1748-
files: []string{"assets/icon.jpeg"},
1749-
expected: "assets/icon.jpeg",
1750-
},
1751-
"assets/icon.gif found": {
1752-
files: []string{"assets/icon.gif"},
1753-
expected: "assets/icon.gif",
1754-
},
1755-
"png wins over gif in assets": {
1756-
files: []string{"assets/icon.png", "assets/icon.gif"},
1757-
expected: "assets/icon.png",
1758-
},
1759-
"jpg wins over gif in assets": {
1760-
files: []string{"assets/icon.jpg", "assets/icon.gif"},
1761-
expected: "assets/icon.jpg",
1762-
},
1763-
"root icon.png found when no assets": {
1764-
files: []string{"icon.png"},
1765-
expected: "icon.png",
1766-
},
1767-
"root icon.jpg found when no assets": {
1768-
files: []string{"icon.jpg"},
1769-
expected: "icon.jpg",
1770-
},
1771-
"assets takes priority over root": {
1772-
files: []string{"assets/icon.gif", "icon.png"},
1773-
expected: "assets/icon.gif",
1774-
},
1775-
"no icon files returns empty": {
1776-
files: []string{},
1777-
expected: "",
1778-
},
1779-
}
1780-
for name, tc := range tests {
1781-
t.Run(name, func(t *testing.T) {
1782-
fs := afero.NewMemMapFs()
1783-
for _, f := range tc.files {
1784-
require.NoError(t, afero.WriteFile(fs, f, []byte("img"), 0o644))
1785-
}
1786-
result := resolveIconPath(fs, tc.manifestIcon)
1787-
assert.Equal(t, tc.expected, result)
1788-
})
1789-
}
1790-
}

‎internal/shared/types/slack_yaml.go‎

Lines changed: 14 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,9 @@ import (
1818
"os"
1919
"path/filepath"
2020

21+
"github.com/slackapi/slack-cli/internal/icon"
2122
"github.com/slackapi/slack-cli/internal/slackerror"
23+
"github.com/spf13/afero"
2224
)
2325

2426
type SlackYaml struct {
@@ -27,33 +29,25 @@ type SlackYaml struct {
2729
Hash string
2830
}
2931

30-
var supportedIconExtensions = []string{".png", ".jpg", ".jpeg", ".gif"}
31-
3232
// hasValidIconPath returns false if icon path is provided but is not valid and true otherwise
33-
func (sy *SlackYaml) hasValidIconPath() bool {
34-
var wd, err = os.Getwd()
35-
if err == nil {
36-
if sy.Icon == "" {
37-
for _, ext := range supportedIconExtensions {
38-
candidate := filepath.Join(wd, "assets", "icon"+ext)
39-
if _, err := os.Stat(candidate); !os.IsNotExist(err) {
40-
sy.Icon = filepath.Join("assets", "icon"+ext)
41-
break
42-
}
43-
}
44-
} else {
45-
if _, err := os.Stat(filepath.Join(wd, sy.Icon)); os.IsNotExist(err) {
46-
return false
47-
}
33+
func (sy *SlackYaml) hasValidIconPath(fs afero.Fs) bool {
34+
wd, err := os.Getwd()
35+
if err != nil {
36+
return true
37+
}
38+
if sy.Icon == "" {
39+
sy.Icon = icon.ResolveIconPath(afero.NewBasePathFs(fs, wd), "")
40+
} else {
41+
if _, err := fs.Stat(filepath.Join(wd, sy.Icon)); os.IsNotExist(err) {
42+
return false
4843
}
4944
}
50-
5145
return true
5246
}
5347

5448
// Verify checks that the app manifest meets some basic requirements
55-
func (sy *SlackYaml) Verify() error {
56-
if !sy.hasValidIconPath() {
49+
func (sy *SlackYaml) Verify(fs afero.Fs) error {
50+
if !sy.hasValidIconPath(fs) {
5751
return slackerror.New("Please specify a valid icon path in app manifest")
5852
}
5953
return nil

‎internal/shared/types/slack_yaml_test.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"path/filepath"
2020
"testing"
2121

22+
"github.com/spf13/afero"
2223
"github.com/stretchr/testify/assert"
2324
"github.com/stretchr/testify/require"
2425
)
@@ -92,7 +93,7 @@ func Test_SlackYaml_hasValidIconPath(t *testing.T) {
9293
defer func() { require.NoError(t, os.Chdir(origDir)) }()
9394

9495
sy := &SlackYaml{Icon: tc.icon}
95-
assert.Equal(t, tc.expected, sy.hasValidIconPath())
96+
assert.Equal(t, tc.expected, sy.hasValidIconPath(afero.NewOsFs()))
9697
})
9798
}
9899
}
@@ -128,9 +129,9 @@ func Test_SlackYaml_Verify(t *testing.T) {
128129

129130
sy := &SlackYaml{Icon: tc.icon}
130131
if tc.expectErr {
131-
assert.Error(t, sy.Verify())
132+
assert.Error(t, sy.Verify(afero.NewOsFs()))
132133
} else {
133-
assert.NoError(t, sy.Verify())
134+
assert.NoError(t, sy.Verify(afero.NewOsFs()))
134135
}
135136
})
136137
}

0 commit comments

Comments
 (0)