-
Notifications
You must be signed in to change notification settings - Fork 837
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.
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
Add Support for App Manifest Endpoints (apps.manifest.*) #1123
Add Support for App Manifest Endpoints (apps.manifest.*) #1123
Conversation
@@ -15,7 +15,7 @@ | |||
|
|||
|
|||
class TestWebClientCoverage(unittest.TestCase): | |||
# 240 endpoints as of Sept 4, 2021 | |||
# 240 endpoints as of Sept 4, 2021 // TODO :: update before merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this relies on the publicly accessible methods, not sure how this is to be changed (or not)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can manually add the new elements to all_api_methods
in the following line of code with a TODO comment saying # TODO: we will delete this once the APIs are publicly available
.
@@ -366,6 +366,23 @@ async def run_method(self, method_name, method, async_method): | |||
method(client_id="111.222", client_secret="xxx")["method"] | |||
) | |||
await async_method(client_id="111.222", client_secret="xxx") | |||
elif method_name == "apps_manifest_create": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've always had issues with running tests locally, but the ones currently failing either are websockets
or sqlite3
related. Can debug further tomorrow, but would appreciate any guidance on how to ensure all is well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sqlite3 errors, perhaps your local environment has an old version of file based databases. You can delete logs/*.db
files to fix the errors. I'm not sure about the error with websockets package but you can try running tests using the scripts under scripts
directory after setting up a plain python venv (python 3 -m venv .venv && source .venv/bin/activate
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your suggestion resolved the issues I was having. Thank you!
@misscoded I just adjusted the GH Actions settings to enable CI builds for PRs toward feat- branches. Can you rebase onto the latest main revision? 0045b5d |
e8b104c
to
8d1b672
Compare
Codecov Report
@@ Coverage Diff @@
## feat-app-manifests #1123 +/- ##
======================================================
+ Coverage 88.05% 88.10% +0.05%
======================================================
Files 110 110
Lines 10681 10735 +54
======================================================
+ Hits 9405 9458 +53
- Misses 1276 1277 +1
Continue to review full report at Codecov.
|
b40bcfd
to
ef8338f
Compare
async def apps_manifest_create( | ||
self, | ||
*, | ||
manifest: str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seratch I want to verify I've crafted these signatures correctly. For testing purposes (and as I mentioned in the corresponding PR), I used the following:
client.apps_manifest_create(token=CONFIGURATION_ACCESS_TOKEN, manifest=VALID_APP_MANIFEST)
Again, since these tokens are so specific to this set of methods, I assume the explicit passing of the tooling token will be often utilized. Does anything need to change about the arguments to encourage this, or is what is here fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, for all methods except api.test
, token
argument is available as part of kwargs. The current code works as-is.
For apps.connections.open API method, we added app_token
this way for consistency with Node SDK. We may want to have something similar for tooling tokens but I think that it's not really necessary so far.
Speaking of realistic use cases, I don't think developers often want to use bot token along with tooling token in a single WebClient instance. When using apps.manifest.* endpoints, it's rare to reuse a WebClient associated with a specific workspace installation (except the case where you would like to send manifest via an existing Slack app; I don't think this is a common use case). For this reason, I think the current code should be fine.
If many people think explicit name of the argument should exist here and we receive such feedback a lot, we may revisit, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good me (I may add integration tests later but it's okay not to have it so far). You can merge this PR if you squash commits into one in advance (or using the button to squash commits).
async def apps_manifest_create( | ||
self, | ||
*, | ||
manifest: str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, for all methods except api.test
, token
argument is available as part of kwargs. The current code works as-is.
For apps.connections.open API method, we added app_token
this way for consistency with Node SDK. We may want to have something similar for tooling tokens but I think that it's not really necessary so far.
Speaking of realistic use cases, I don't think developers often want to use bot token along with tooling token in a single WebClient instance. When using apps.manifest.* endpoints, it's rare to reuse a WebClient associated with a specific workspace installation (except the case where you would like to send manifest via an existing Slack app; I don't think this is a common use case). For this reason, I think the current code should be fine.
If many people think explicit name of the argument should exist here and we receive such feedback a lot, we may revisit, though.
Hi everyone, awesome that you are adding support for this! Any idea on when it would be merged? I'm planning on using it to automagically change for example the slash command url with the latest ngrok link for local development. Will this be possible? The only thing I don't understand yet are the access tokens, they seems to only be valid for 12 hours? Thanks so much! |
Hi @AntonVanGoethem -- love that you're excited about this new functionality! Your use case sounds compelling, and though I've never tried it myself, I don't see why it wouldn't be possible. In terms of availability, as this feature is still in beta, we'll be cutting a dedicated beta release here shortly, so stay tuned. 🙂 In the meantime, if you'd like to learn more about access tokens, I'd recommend checking out the documentation on these new app configuration tokens. I can confirm that they do expire after 12 hours, at which point (or ideally before) you need to refresh the token. To refresh config tokens, a call can be made to Hope this helps! |
Okay, thanks a lot! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any updates on this "beta release"?
Summary
Fixes #1119
This pull request introduces support for the App Manifest endpoints, including:
apps.manifest.create
apps.manifest.delete
apps.manifest.export
apps.manifest.update
apps.manifest.validate
tooling.tokens.rotate
Category (place an
x
in each of the[ ]
)/docs-src
(Documents, have you run./scripts/docs.sh
?)/docs-src-v2
(Documents, have you run./scripts/docs-v2.sh
?)/tutorial
(PythOnBoardingBot tutorial)tests
/integration_tests
(Automated tests for this library)Requirements (place an
x
in each[ ]
)python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.sh
after making the changes.