Add support for apps.manifest.* endpoints #1690
Conversation
misscoded
added
semver:minor
enhancement
Co-authored-by: Kazuhiro Sera <[email protected]>
| options.slackApiUrl = process.env.SLACK_SDK_TEST_DEV_API_URL; | ||
| } | ||
|
|
||
| const client = new WebClient(BOT_TOKEN, options); |
There was a problem hiding this comment.
We should verify if a WebClient instance can work without a bot token in this scenario.
There was a problem hiding this comment.
I think the token used for BOT_TOKEN is irrelevant for the testing of the endpoints since they rely on the tooling token override, correct? The endpoints will not work with a non-tooling token, if that's what you mean.
There was a problem hiding this comment.
Yes, you're right. The bot token is not necessary at all. Removing it makes the code more realistic and we should demonstrate how to use the API client usually.
There was a problem hiding this comment.
Removing it makes the code more realistic
Won't most folks using the manifest API have a client that's instantiated using a bot token (or user token)? Happy to remove it, but want to clarify we're on the same page.
There was a problem hiding this comment.
When it comes to a bolt app, yes it is. However, in this case, a developer using the client for the app manifest scenario may not have any bot token, right? (say, starting a new app development). Indeed, a developer may initialize the client with a bot token (as part of a larger code base or due to whatever reasons), but this assuption does not cover all use cases. The e2e test code should make sure if it works for most common use cases. Regarding the APIs we're working on now, I believe the primary one is without a bot token, and then secoundary could be with a bot token. Does this make sense to you?
There was a problem hiding this comment.
Just approved. Once this integration test is improved, go ahead with merging this PR 👍
|
FYI I have cherry-picked this merged commit to the |
Summary
Fixes #1340
Requirements (place an
xin each[ ])