-
Notifications
You must be signed in to change notification settings - Fork 836
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
Google Cloud Storage OAuth backend #1628
base: main
Are you sure you want to change the base?
Google Cloud Storage OAuth backend #1628
Conversation
Since the time I wrote the original code, I saw there are a couple additions to the other installation stores (like the S3 one) compared to what I did, like historical data and more situations to handle when finding and deleting installs. Let me know what are your thoughts on the proposed changes and if we should tackle those too in this PR 🙂 |
Hi @ccaruceru, thanks for sending this pull request! As you can see here, all the existing built-in installation store implementations pass the same set of scenario tests. If we accept a new one, it must pass the same tests. I'm not sure if you have time or are willing to spend more time, but adding complete test assets to ensure the same quality would be appreciated. |
hi @seratch! I'll have a look at the tests this week and see if I can come up with something similar 👌🏻 |
@seratch I added the tests as you asked and modified the |
Args: | ||
installation (Installation): information about the user and the app usage authorization | ||
""" | ||
self.save(installation) |
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 could be wrong, but to make this really async you should use for async methods gcloud-aio-storage
package that supports async GCS as there is no support for it in original Google Cloud Storage package:
https://talkiq.github.io/gcloud-aio/autoapi/storage/index.html
This is to all async methods here - but also question to other storages (File, S3, SQLite etc.) that are doing this and they aren't trully async as they just use synchronous methods. SQLite for example needs to use aiosqlite
for async methods to be trully async.
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.
Thank's for pointing this out. It's true that this implementation doesn't provide true async support because it relies on the synchronous methods of the official google-cloud-storage
package. However, one of the goals here is to unblock users who need to work with the async version of the app and still support GCS.
Looking at gcloud-aio-storage
, it does offer proper async methods but it lacks support for certain features like the max_results
parameter when doing list_blobs
that's used inside the InstallationStore.
I agree that the asynchronous aspect of the app as a whole should be addressed, but in a future PR to ensure a more consistent approach across all backend/storage providers.
Summary
Add Google Cloud Storage (GCS) support for OAuth installation and state stores, as a follow up from slackapi/bolt-python#962 (comment).
(ported from ccaruceru/slack-multireact/multi_reaction_add/oauth)
Testing
e.g.
/slack/install
pagebot*
andinstaller*
files are createdbot
andinstaller
files are unchangedCategory
/docs
(Documents)/tutorial
(PythOnBoardingBot tutorial)tests
/integration_tests
(Automated tests for this library)Requirements
python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.sh
after making the changes.