Skip to content

feat: 'app link' supports run-on-slack apps with local manifest source #56

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mwbrooks merged 6 commits into from
Apr 22, 2025
+176 −7
Merged

feat: 'app link' supports run-on-slack apps with local manifest source #56

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
471a848
feat: update 'app link' to support run-on-slack apps with local manif…
mwbrooks Apr 21, 2025
9a5be4c
test: fix failing 'project init' test
mwbrooks Apr 21, 2025
cec1f5f
Merge branch 'main' into mbrooks-app-link-rosi
mwbrooks Apr 21, 2025
ce180eb
test: update tests with latest ctx changes
mwbrooks Apr 22, 2025
30e69a7
refactor: simplify the manifest update conditional
mwbrooks Apr 22, 2025
5f30426
Merge branch 'main' into mbrooks-app-link-rosi
mwbrooks Apr 22, 2025
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
10 changes: 7 additions & 3 deletions cmd/app/link.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ func LinkCommandRunE(ctx context.Context, clients *shared.ClientFactory, app *ty
// explaining how to link apps, in case the user declines.
func LinkAppHeaderSection(ctx context.Context, clients *shared.ClientFactory, shouldConfirm bool) {
var secondaryText = []string{
"Add an existing app created on app settings",
"Add an existing app from app settings",
Copy link
Copy Markdown
Member Author

@mwbrooks mwbrooks Apr 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: App Settings now displays Slack CLI created apps as read-only, un-clickable. So, I figured we should update this text to not be restricted to "created on app settings" because the app may be created by the CLI.

Copy link
Copy Markdown
Member

@zimeg zimeg Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⭐ What an eye for good readings!

"Find your existing apps at: " + style.Underline("https://api.slack.com/apps"),
}

Expand Down Expand Up @@ -156,10 +156,14 @@ func LinkExistingApp(ctx context.Context, clients *shared.ClientFactory, app *ty
}
}

// Confirm to update manifest source to remote
// Confirm to update manifest source to remote.
// - Update the manifest source to remote when its a GBP project with a local manifest.
// - Do not update manifest source for ROSI projects, because they can only be local manifests.
manifestSource, err := clients.Config.ProjectConfig.GetManifestSource(ctx)
isManifestSourceRemote := manifestSource.Equals(config.MANIFEST_SOURCE_REMOTE)
isSlackHostedProject := cmdutil.IsSlackHostedProject(ctx, clients) == nil

if !manifestSource.Equals(config.MANIFEST_SOURCE_REMOTE) || err != nil {
if err != nil || (!isManifestSourceRemote && !isSlackHostedProject) {
// When undefined, the default is config.MANIFEST_SOURCE_LOCAL
if !manifestSource.Exists() {
manifestSource = config.MANIFEST_SOURCE_LOCAL
Expand Down
167 changes: 163 additions & 4 deletions cmd/app/link_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"testing"

"github.com/slackapi/slack-cli/internal/api"
"github.com/slackapi/slack-cli/internal/app"
"github.com/slackapi/slack-cli/internal/config"
"github.com/slackapi/slack-cli/internal/iostreams"
"github.com/slackapi/slack-cli/internal/shared"
Expand Down Expand Up @@ -430,7 +431,7 @@ func Test_Apps_Link(t *testing.T) {
CmdArgs: []string{},
ExpectedError: slackerror.New(slackerror.ErrAppNotFound),
},
"accept manifest source prompt and saves information about the provided deployed app": {
"accepting manifest source prompt should save information about the provided deployed app": {
Copy link
Copy Markdown
Member Author

@mwbrooks mwbrooks Apr 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: Just re-wording after reading over the test cases for clarity.

Copy link
Copy Markdown
Member

@zimeg zimeg Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

: {

Copy link
Copy Markdown
Member Author

@mwbrooks mwbrooks Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

: }

Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
cm.AuthInterface.On("Auths", mock.Anything).Return([]types.SlackAuth{
mockLinkSlackAuth2,
Expand Down Expand Up @@ -503,7 +504,7 @@ func Test_Apps_Link(t *testing.T) {
)
},
},
"decline manifest source prompt should not link app": {
"declining manifest source prompt should not link app": {
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
cm.AddDefaultMocks()
setupAppLinkCommandMocks(t, ctx, cm, cf)
Expand Down Expand Up @@ -537,6 +538,159 @@ func Test_Apps_Link(t *testing.T) {
require.Len(t, apps, 0)
},
},
"manifest source prompt should not display for Run-on-Slack apps with local manifest source": {
Copy link
Copy Markdown
Member Author

@mwbrooks mwbrooks Apr 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: New test case to assert that the manifest update prompt is NOT displayed for ROSI apps with a local manifest. I don't check for remote manifests, because that's handled in an existing test-case.

Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
cm.AuthInterface.On("Auths", mock.Anything).Return([]types.SlackAuth{
mockLinkSlackAuth1,
mockLinkSlackAuth2,
}, nil)
cm.AddDefaultMocks()
setupAppLinkCommandMocks(t, ctx, cm, cf)
// Set manifest source to local
if err := cm.Config.ProjectConfig.SetManifestSource(ctx, config.MANIFEST_SOURCE_LOCAL); err != nil {
require.FailNow(t, fmt.Sprintf("Failed to set the manifest source in the memory-based file system: %s", err))
}
// Mock manifest for Run-on-Slack app
manifestMock := &app.ManifestMockObject{}
manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything).Return(types.SlackYaml{
AppManifest: types.AppManifest{
Settings: &types.AppSettings{
FunctionRuntime: types.SLACK_HOSTED,
},
},
}, nil)
cf.AppClient().Manifest = manifestMock
cm.IO.On("SelectPrompt",
mock.Anything,
"Select the existing app team",
mock.Anything,
mock.Anything,
mock.Anything,
).Return(iostreams.SelectPromptResponse{
Flag: true,
Option: mockLinkSlackAuth1.TeamDomain,
}, nil)
cm.IO.On("InputPrompt",
mock.Anything,
"Enter the existing app ID",
mock.Anything,
).Return(mockLinkAppID1, nil)
cm.IO.On("SelectPrompt",
mock.Anything,
"Choose the app environment",
mock.Anything,
mock.Anything,
mock.Anything,
).Return(iostreams.SelectPromptResponse{
Flag: true,
Option: "deployed",
}, nil)
cm.ApiInterface.On(
"GetAppStatus",
mock.Anything,
mockLinkSlackAuth1.Token,
[]string{mockLinkAppID1},
mockLinkSlackAuth1.TeamID,
).Return(api.GetAppStatusResult{}, nil)
},
CmdArgs: []string{},
ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {
expectedApp := types.App{
AppID: mockLinkAppID1,
TeamDomain: mockLinkSlackAuth1.TeamDomain,
TeamID: mockLinkSlackAuth1.TeamID,
EnterpriseID: mockLinkSlackAuth1.EnterpriseID,
}
actualApp, err := cm.AppClient.GetDeployed(
ctx,
mockLinkSlackAuth1.TeamID,
)
require.NoError(t, err)
assert.Equal(t, expectedApp, actualApp)
// Assert manifest confirmation prompt was not displayed
cm.IO.AssertNotCalled(t, "ConfirmPrompt",
mock.Anything,
LinkAppManifestSourceConfirmPromptText,
mock.Anything,
)
},
},
"manifest source prompt should display for GBP apps with local manifest source": {
Copy link
Copy Markdown
Member Author

@mwbrooks mwbrooks Apr 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: Added a test-case to assert that the manifest update prompt is still displayed for GBP apps with a local manifest. This case might be redundant, but I felt high confidence after adding it.

Copy link
Copy Markdown
Member

@zimeg zimeg Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding it! I had a good feeling when running the dev build and reading code, but this will be so helpful in guarding against unexpected updates to these prompts.

The assert prompting at the end is very very good for this 🏆 ✨

Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
cm.AuthInterface.On("Auths", mock.Anything).Return([]types.SlackAuth{
mockLinkSlackAuth1,
mockLinkSlackAuth2,
}, nil)
cm.AddDefaultMocks()
setupAppLinkCommandMocks(t, ctx, cm, cf)
// Set manifest source to local
if err := cm.Config.ProjectConfig.SetManifestSource(ctx, config.MANIFEST_SOURCE_LOCAL); err != nil {
require.FailNow(t, fmt.Sprintf("Failed to set the manifest source in the memory-based file system: %s", err))
}
// Mock manifest for Run-on-Slack app
manifestMock := &app.ManifestMockObject{}
manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything).Return(types.SlackYaml{}, nil)
cf.AppClient().Manifest = manifestMock
cm.IO.On("ConfirmPrompt",
mock.Anything,
LinkAppManifestSourceConfirmPromptText,
mock.Anything,
).Return(true, nil)
cm.IO.On("SelectPrompt",
mock.Anything,
"Select the existing app team",
mock.Anything,
mock.Anything,
mock.Anything,
).Return(iostreams.SelectPromptResponse{
Flag: true,
Option: mockLinkSlackAuth1.TeamDomain,
}, nil)
cm.IO.On("InputPrompt",
mock.Anything,
"Enter the existing app ID",
mock.Anything,
).Return(mockLinkAppID1, nil)
cm.IO.On("SelectPrompt",
mock.Anything,
"Choose the app environment",
mock.Anything,
mock.Anything,
mock.Anything,
).Return(iostreams.SelectPromptResponse{
Flag: true,
Option: "deployed",
}, nil)
cm.ApiInterface.On(
"GetAppStatus",
mock.Anything,
mockLinkSlackAuth1.Token,
[]string{mockLinkAppID1},
mockLinkSlackAuth1.TeamID,
).Return(api.GetAppStatusResult{}, nil)
},
CmdArgs: []string{},
ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {
expectedApp := types.App{
AppID: mockLinkAppID1,
TeamDomain: mockLinkSlackAuth1.TeamDomain,
TeamID: mockLinkSlackAuth1.TeamID,
EnterpriseID: mockLinkSlackAuth1.EnterpriseID,
}
actualApp, err := cm.AppClient.GetDeployed(
ctx,
mockLinkSlackAuth1.TeamID,
)
require.NoError(t, err)
assert.Equal(t, expectedApp, actualApp)
// Assert manifest confirmation prompt was displayed
cm.IO.AssertCalled(t, "ConfirmPrompt",
mock.Anything,
LinkAppManifestSourceConfirmPromptText,
mock.Anything,
)
},
},
}, func(clients *shared.ClientFactory) *cobra.Command {
clients.SDKConfig.WorkingDirectory = "."
return NewLinkCommand(clients)
Expand All @@ -552,7 +706,7 @@ func Test_Apps_LinkAppHeaderSection(t *testing.T) {
"When shouldConfirm is false": {
shouldConfirm: false,
expectedOutputs: []string{
"Add an existing app created on app settings",
"Add an existing app from app settings",
"Find your existing apps at: https://api.slack.com/apps",
},
unexpectedOutputs: []string{
Expand All @@ -562,7 +716,7 @@ func Test_Apps_LinkAppHeaderSection(t *testing.T) {
"When shouldConfirm is true": {
shouldConfirm: true,
expectedOutputs: []string{
"Add an existing app created on app settings",
"Add an existing app from app settings",
"Find your existing apps at: https://api.slack.com/apps",
"Manually add apps later with",
},
Expand Down Expand Up @@ -609,4 +763,9 @@ func setupAppLinkCommandMocks(t *testing.T, ctx context.Context, cm *shared.Clie
if err := cm.Config.ProjectConfig.SetManifestSource(ctx, config.MANIFEST_SOURCE_REMOTE); err != nil {
require.FailNow(t, fmt.Sprintf("Failed to set the manifest source in the memory-based file system: %s", err))
}

// Mock manifest
manifestMock := &app.ManifestMockObject{}
manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything).Return(types.SlackYaml{}, nil)
cf.AppClient().Manifest = manifestMock
Comment on lines +767 to +770
Copy link
Copy Markdown
Member

@zimeg zimeg Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Perhaps personal preference, but I'm curious what you think about:

Having standalone and shared "setup"..."mock" functions I find to be confusing, even if convenient. IMO inlining mocks is super helpful in table tests to keep each unit separate.

No blocker at all for this PR, but does this seem like a longterm idea we should nudge towards?

Copy link
Copy Markdown
Member Author

@mwbrooks mwbrooks Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧠 I agree with you. I also find the .DefaultMocks() method confusing. Sometimes it can be very convenient because I just need a standard happy path mock, but often tests don't require it. The same can be true for the "setup" helpers, where they become overloaded with defaults that can actually drift some of the tests away from their target.

It would be nice to settle on some practice that keeps the tests short(er) while explicit.

}
6 changes: 6 additions & 0 deletions cmd/project/init_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (

"github.com/slackapi/slack-cli/cmd/app"
"github.com/slackapi/slack-cli/internal/api"
internalApp "github.com/slackapi/slack-cli/internal/app"
"github.com/slackapi/slack-cli/internal/iostreams"
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/shared/types"
Expand Down Expand Up @@ -237,4 +238,9 @@ func setupProjectInitCommandMocks(t *testing.T, ctx context.Context, cm *shared.
if err := cm.Fs.MkdirAll(filepath.Dir(projectDirPath), 0755); err != nil {
require.FailNow(t, fmt.Sprintf("Failed to create the directory %s in the memory-based file system", projectDirPath))
}

// Mock manifest
manifestMock := &internalApp.ManifestMockObject{}
manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything).Return(types.SlackYaml{}, nil)
cf.AppClient().Manifest = manifestMock
}
Loading