Skip to content
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 Google Cloud Functions adapter (ref #646) #649

Merged
merged 4 commits into from
May 18, 2022

Conversation

seratch
Copy link
Member

@seratch seratch commented May 16, 2022

This pull request adds a new adapter for Google Cloud Functions users. You can use Flask adapter for it as long as you don't have the OAuth flow in your app. However, as we see at #646, implementing the OAuth flow is far different and it would be confusing for developers. This pull request adds a new adapter that is quite similar to the AWS Lambda adapter.

One thing developers should be aware of would be the lazy listeners are not yet available for Google Cloud Functions, unlike AWS Lambda. Thus, I've added Noop runner, which just raises an exception when a developer tries to use lazy listeners in Cloud Functions app. Also, this is an improvement from the Flask adapter way.

Category (place an x in each of the [ ])

  • slack_bolt.App and/or its core components
  • slack_bolt.async_app.AsyncApp and/or its core components
  • Adapters in slack_bolt.adapter
  • Document pages under /docs
  • Others

Requirements (place an x in each [ ])

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.

  • 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.
  • I've run ./scripts/install_all_and_run_tests.sh after making the changes.

@seratch seratch added enhancement New feature or request area:adapter labels May 16, 2022
@seratch seratch added this to the 1.14.0 milestone May 16, 2022
@seratch seratch self-assigned this May 16, 2022
@codecov
Copy link

codecov bot commented May 16, 2022

Codecov Report

Merging #649 (632a03d) into main (5d3ee55) will decrease coverage by 0.02%.
The diff coverage is 87.09%.

❗ Current head 632a03d differs from pull request most recent head 4bdfd1a. Consider uploading reports for the commit 4bdfd1a to get more accurate results

@@            Coverage Diff             @@
##             main     #649      +/-   ##
==========================================
- Coverage   92.07%   92.05%   -0.03%     
==========================================
  Files         170      172       +2     
  Lines        5833     5864      +31     
==========================================
+ Hits         5371     5398      +27     
- Misses        462      466       +4     
Impacted Files Coverage Δ
...ack_bolt/adapter/google_cloud_functions/handler.py 86.20% <86.20%> (ø)
...ck_bolt/adapter/google_cloud_functions/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d3ee55...4bdfd1a. Read the comment docs.

@seratch
Copy link
Member Author

seratch commented May 16, 2022

Ah, yeah I should add some tests. Will do after finishing other tasks.


def handle(self, req: Request) -> Response:
if req.method == "GET":
if self.app.oauth_flow is not None:

Choose a reason for hiding this comment

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

We can remove this extra if

Just change

        if req.method == "GET":
            if self.app.oauth_flow is not None:

to

        if req.method == "GET" and self.app.oauth_flow is not None

Copy link
Member Author

Choose a reason for hiding this comment

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

@stasfilin Thanks for the suggestion!

@seratch seratch requested review from filmaj and mwbrooks May 16, 2022 22:38
@seratch
Copy link
Member Author

seratch commented May 16, 2022

Updated the code (thanks for the suggestion, @stasfilin!) and added unit tests ✅

Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Code LGTM but I did not create a test app using this as salesforce does not allow me to create Google Cloud projects 😑

def endpoint():
return SlackRequestHandler(app).handle(request)

with flask_app.test_client() as client:
Copy link
Contributor

Choose a reason for hiding this comment

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

I love this pattern.



# Step1: Create a new Slack App: https://api.slack.com/apps
# Bot Token Scopes: chat:write, commands, app_mentions:read
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI the .env.yaml.oauth-sample has a different list than this; it includes channels.history.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I've updated the part.

@seratch seratch merged commit 0a2cee6 into slackapi:main May 18, 2022
@seratch seratch deleted the google-cloud-functions-adapter branch May 18, 2022 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:adapter enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants