Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
36 changes: 22 additions & 14 deletions internal/pkg/apps/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ package apps
import (
"context"
"fmt"
"os"
"path/filepath"
"strings"
"time"

Expand All @@ -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
Expand All @@ -40,6 +41,23 @@ const (

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

var supportedIconExtensions = []string{".png", ".jpg", ".jpeg", ".gif"}
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: Just curious, have you tested all of these images types? I just want to make sure that the API supports each.


func resolveIconPath(fs afero.Fs, manifestIcon string) 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.

suggestion: Thoughts on moving this to internal/icon/icon.go or maybe internal/config/icon.go. My preference is the first to avoid cluttering our already messy package config.

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
}
}
Comment on lines +50 to +56
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: It looks like the implementation will attempt to find the first image that matches the file extensions and use it when there is no path defined?

}
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")
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
64 changes: 64 additions & 0 deletions internal/pkg/apps/install_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
})
}
}
14 changes: 9 additions & 5 deletions internal/shared/types/slack_yaml.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Comment on lines +37 to +42
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: Since we're using this loop logic in 2 places, I think it would make sense to implement it as a function in internal/icon/icon.go.

}
} else {
if _, err := os.Stat(filepath.Join(wd, sy.Icon)); os.IsNotExist(err) {
Expand Down
25 changes: 25 additions & 0 deletions internal/shared/types/slack_yaml_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {},
Expand Down