feat: 'app link' supports run-on-slack apps with local manifest source by mwbrooks · Pull Request #56 · slackapi/slack-cli

mwbrooks · 2025-04-21T23:30:01Z

Summary

This pull request updates the app link command to work with run-on-Slack apps that have a local manifest source.

📝 1 line change and the rest are test-cases.

Perhaps, this as the beginning of loosening up the app link command to not always require a remote manifest source.

This work came about from the GitHub Issue slackapi/deno-slack-sdk#451. In the issue, I wasn't able to suggest using the app link command because it would force the Deno SDK app to have a remote manifest. This seemed silly, since all Deno SDK apps have a local manifest. Since developer can now see their Deno SDK apps in https://api.slack.com/apps, we should allow them to link the app in their project.

Preview

Deno SDK

Should not display the manifest source warning prompt and should keep manifest source as local

2025-04-21-app-link-deno.mov

Bolt for JavaScript

Should display the manifest source warning prompt

2025-04-21-app-link-gbp.mov

Reviewers

# Test Deno SDK
$ lack create asdf/ -t slack-samples/deno-starter-template
$ cd asdf/
$ lack app link
# → Confirm NO manifest source prompt
# → CTRL+C (or, extra mile if you want to add an App ID and confirm it's still a local manifest)
# Update manifest source to be remote
$ vim .slack/config.json
# → Change "local" to "remote"
$ lack app link
# → Confirm NO manifest source prompt
# → CTRL+C
$ cd ../
$ rm -rf asdf/

# Test Bolt for JavaScript
$ lack create asdf/ -t slack-samples/bolt-js-custom-function-template
$ cd asdf/
$ lack app link
# → Confirm manifest source prompt
# → CTRL+C
# Update manifest source to be remote
$ vim .slack/config.json
# → Change "local" to "remote"
$ lack app link
# → Confirm NO manifest source prompt
# → CTRL+C
$ cd ../
$ rm -rf asdf/

Requirements

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

…est src

mwbrooks

✏️ A few notes for the reviewer, so the test-cases don't look overwhelming!

mwbrooks · 2025-04-21T23:31:05Z

 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",


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.

⭐ What an eye for good readings!

mwbrooks · 2025-04-21T23:32:08Z

 	manifestSource, err := clients.Config.ProjectConfig.GetManifestSource(ctx)

-	if !manifestSource.Equals(config.MANIFEST_SOURCE_REMOTE) || err != nil {
+	if err != nil || (!manifestSource.Equals(config.MANIFEST_SOURCE_REMOTE) && cmdutil.IsSlackHostedProject(ctx, clients) != nil) {


note: This is the main change. We only want to display the manifest source update prompt when the manifest is not already remote AND it's not a ROSI app.

Nit: I am perhaps low on brainpower right now, but this is not so obvious to me...

At the expense of an extra line or two, what do you think about a separate variable for:

isSlackHostedProject := cmdutil.IsSlackHostedProject(ctx, clients) == nil

You and I were both low on brain power. I originally separated it to a variable in order to make sense of the conditional. Then I combined it afterwards. I'm happy to separate it out and I may do the same with the remote manifest conditional because the ! and .Equals take a moment to make sense.

Commt 30e69a7 takes your suggestion to heart and it's so much better!

isManifestSourceRemote := manifestSource.Equals(config.MANIFEST_SOURCE_REMOTE) isSlackHostedProject := cmdutil.IsSlackHostedProject(ctx, clients) == nil if err != nil || (!isManifestSourceRemote && !isSlackHostedProject) {

mwbrooks · 2025-04-21T23:32:42Z

 			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": {


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

mwbrooks · 2025-04-21T23:33:37Z

 				require.Len(t, apps, 0)
 			},
 		},
+		"manifest source prompt should not display for Run-on-Slack apps with local manifest source": {


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.

mwbrooks · 2025-04-21T23:34:35Z

+				)
+			},
+		},
+		"manifest source prompt should display for GBP apps with local manifest source": {


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.

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 🏆 ✨

mwbrooks · 2025-04-21T23:35:02Z

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


note: Just a little reflection on how we don't actually have a nice way to mock the Manifest client atm. We have a ManifestMockObject but it's not automatically setup in the clients mock.

codecov · 2025-04-22T00:02:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.89%. Comparing base (4cfcf8f) to head (5f30426).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #56      +/-   ##
==========================================
- Coverage   62.89%   62.89%   -0.01%     
==========================================
  Files         210      210              
  Lines       22154    22156       +2     
==========================================
  Hits        13934    13934              
- Misses       7132     7134       +2     
  Partials     1088     1088

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

zimeg

@mwbrooks Thanks tons for tackling the initial issue with careful resolution and also finding a fix for future encounters! 💌

I got a bit stumped on logic within, but all testing is working well so marking this as approved 🚢 💨

In following comments I left a note on the confusion I had and another thought about testing. Please feel free to ignore and merge or respond if that feels right 👾

zimeg · 2025-04-22T00:54:24Z

 	manifestSource, err := clients.Config.ProjectConfig.GetManifestSource(ctx)

-	if !manifestSource.Equals(config.MANIFEST_SOURCE_REMOTE) || err != nil {
+	if err != nil || (!manifestSource.Equals(config.MANIFEST_SOURCE_REMOTE) && cmdutil.IsSlackHostedProject(ctx, clients) != nil) {


Nit: I am perhaps low on brainpower right now, but this is not so obvious to me...

At the expense of an extra line or two, what do you think about a separate variable for:

isSlackHostedProject := cmdutil.IsSlackHostedProject(ctx, clients) == nil

zimeg · 2025-04-22T00:55:05Z

 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",


⭐ What an eye for good readings!

zimeg · 2025-04-22T00:57:42Z

+				)
+			},
+		},
+		"manifest source prompt should display for GBP apps with local manifest source": {


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 🏆 ✨

zimeg · 2025-04-22T01:00:27Z

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


📝 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?

🧠 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.

mwbrooks · 2025-04-22T17:34:59Z

Thanks for the quick review @zimeg! I totally agree with your "nit" comment to simplify the conditional logic. Thanks a bunch for calling that out and commit 30e69a7 tightened it up! 🔩

feat: update 'app link' to support run-on-slack apps with local manif…

471a848

…est src

mwbrooks added enhancement M-T: A feature request for new functionality changelog Use on updates to be included in the release notes semver:minor Use on pull requests to describe the release version increment labels Apr 21, 2025

mwbrooks added this to the Next Release milestone Apr 21, 2025

mwbrooks self-assigned this Apr 21, 2025

mwbrooks commented Apr 21, 2025

View reviewed changes

mwbrooks added 3 commits April 21, 2025 16:47

test: fix failing 'project init' test

9a5be4c

Merge branch 'main' into mbrooks-app-link-rosi

cec1f5f

test: update tests with latest ctx changes

ce180eb

mwbrooks marked this pull request as ready for review April 22, 2025 00:07

mwbrooks requested a review from a team as a code owner April 22, 2025 00:07

zimeg approved these changes Apr 22, 2025

View reviewed changes

mwbrooks added 2 commits April 22, 2025 10:24

refactor: simplify the manifest update conditional

30e69a7

Merge branch 'main' into mbrooks-app-link-rosi

5f30426

mwbrooks merged commit 8ec404c into main Apr 22, 2025
6 checks passed

mwbrooks deleted the mbrooks-app-link-rosi branch April 22, 2025 17:36

Conversation

mwbrooks commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Preview

Deno SDK

Bolt for JavaScript

Reviewers

Requirements

Uh oh!

mwbrooks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwbrooks Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zimeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwbrooks commented Apr 22, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mwbrooks commented Apr 21, 2025 •

edited

Loading

mwbrooks Apr 21, 2025 •

edited

Loading

codecov Bot commented Apr 22, 2025 •

edited

Loading