Skip to content

Add support for lazy listeners when running with chalice local - #270

Merged
seratch merged 5 commits into
slackapi:mainfrom
jlujan-invitae:jlujan/support-lazy-chalice-cli-local
Mar 30, 2021
Merged

seratch merged 5 commits into
slackapi:mainfrom
jlujan-invitae:jlujan/support-lazy-chalice-cli-local

Conversation

@jlujan-invitae

Copy link
Copy Markdown
Contributor

Addresses the underlying issue in #267.� Add support to ChaliceSlackRequestHandler for lazy listeners when running through chalice local . The current method uses boto('lambda').invoke to invoke an actual lambda function which does not work when developing or running locally with chalice local. This PR implements LocalLambdaClient sub-classed from chalice.test.BaseClient with overload of invoke to match boto's capitalized function signature.

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

@CLAassistant

CLAassistant commented Mar 25, 2021 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@codecov

codecov Bot commented Mar 25, 2021 •

Copy link
Copy Markdown

Codecov Report

Merging #270 (e604bab) into main (57fdb45) will increase coverage by 0.24%.
The diff coverage is 100.00%.

❗ Current head e604bab differs from pull request most recent head a497fac. Consider uploading reports for the commit a497fac to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #270      +/-   ##
==========================================
+ Coverage   91.25%   91.50%   +0.24%     
==========================================
  Files         160      160              
  Lines        4983     5000      +17     
==========================================
+ Hits         4547     4575      +28     
+ Misses        436      425      -11     
Impacted Files Coverage Δ
slack_bolt/adapter/aws_lambda/chalice_handler.py 89.61% <100.00%> (+2.94%) ⬆️
slack_bolt/listener/thread_runner.py 93.39% <0.00%> (+3.77%) ⬆️
...adapter/aws_lambda/chalice_lazy_listener_runner.py 94.73% <0.00%> (+36.84%) ⬆️

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 57fdb45...a497fac. Read the comment docs.

@seratch seratch added area:adapter enhancement New feature or request labels Mar 25, 2021
@seratch seratch added this to the 1.5.0 milestone Mar 25, 2021
@seratch

seratch commented Mar 25, 2021

Copy link
Copy Markdown
Contributor

@jlujan-invitae Thanks for taking the time to make this improvement! The changes already look great to me 👍

One thing I wanted to know is the possibility to have some tests for the pattern in https://github.com/slackapi/bolt-python/blob/main/tests/adapter_tests/aws/test_aws_chalice.py .

If we can add a basic-level (in other words, normal patterns only) unit test emulating chalice local behaviors, it'd be valuable for its maintenance. If some issues or limitation prevent us from having tests for this, it's okay for now. Thoughts?

@jlujan-invitae

Copy link
Copy Markdown
Contributor Author

Sure thing. I will attempt to write tests to cover the additions. Might take me a few days with my current schedule.

@jlujan-invitae

Copy link
Copy Markdown
Contributor Author

@seratch, I think this is in good shape. Might give it a once over. Looks like it improved coverage some.

@seratch

seratch commented Mar 30, 2021

Copy link
Copy Markdown
Contributor

@jlujan-invitae Thanks! The added test looks great to me 👍

@seratch
seratch merged commit 2e32d1b into slackapi:main Mar 30, 2021
jlujan-invitae added a commit to jlujan-invitae/bolt-python that referenced this pull request Apr 28, 2021
* Refactor LocalLambdaClient into seperate file
* try/catch import when running from a deployed lambda
seratch pushed a commit that referenced this pull request Apr 30, 2021
* Fix chalice deployment bug introduced in #270
* Refactor LocalLambdaClient into seperate file
* try/catch import when running from a deployed lambda

* Updates from PR feedback, improve lazy lambda tests
* Only import LocalLambdaClient if CLI and client not passed in
* Add unittest for default lazy listener

* Fix name of mocked lambda client in test_lazy_listeners_non_cli
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