Fix chalice deployment bug introduced in #270 #316
Conversation
* Refactor LocalLambdaClient into seperate file * try/catch import when running from a deployed lambda
Codecov Report
@@ Coverage Diff @@
## main #316 +/- ##
==========================================
- Coverage 91.49% 91.46% -0.03%
==========================================
Files 166 167 +1
Lines 5291 5298 +7
==========================================
+ Hits 4841 4846 +5
- Misses 450 452 +2
Continue to review full report at Codecov.
|
| @@ -18,6 +18,7 @@ | |||
| ChaliceSlackRequestHandler, | |||
| not_found, | |||
| ) | |||
| from slack_bolt.adapter.aws_lambda.local_lambda_client import LocalLambdaClient | |||
There was a problem hiding this comment.
LocalLambdaClient is not directly used in this test file. If you have some intention to have this here, can you add comments telling the reason why we should have this import.
|
|
||
|
|
||
| class ChaliceSlackRequestHandler: | ||
| def __init__(self, app: App, chalice: Chalice): # type: ignore | ||
| def __init__(self, app: App, chalice: Chalice, lambda_client: Optional[Any] = None): # type: ignore |
There was a problem hiding this comment.
Can we have a bit more specific type than Optional[Any] here?
There was a problem hiding this comment.
The actual type is runtime generated by botocore from an API spec. It has a base of botocore.client.BaseClient. I will use that. However, it provides no real type information. Will also add a docstring with more details.
There was a problem hiding this comment.
Looks good to me. I was also thinking we can use BaseClient at least.
| try: | ||
| from slack_bolt.adapter.aws_lambda.local_lambda_client import LocalLambdaClient | ||
| except ImportError: | ||
| LocalLambdaClient = None |
There was a problem hiding this comment.
How about having a global variable like local_lambda_available value and moving the initial import of LocalLambdaClient to ChaliceSlackRequestHandler constructor? As Chalice apps have overhead for loading, I would like to avoid extra loading as much as possible.
…vitae/gh315/fix-chalice-deployment
* Only import LocalLambdaClient if CLI and client not passed in * Add unittest for default lazy listener
|
I identified how this bug went undetected and wrote a description in an issue with the aws/chalice project. The gist is that I was using a specific git revision from my fork in our requirements.txt. Apparently, the chalice packer ignores source distribution and attempts to download a binary distribution by package name and version. Since my fork was pre-release, it detected the previous slack-bolt version and downloaded the available wheel from PyPI. Relevant issue is here aws/chalice#1700 (comment) |
| with mock.patch.dict(os.environ, {"AWS_CHALICE_CLI_MODE": "false"}): | ||
| assert os.environ.get("AWS_CHALICE_CLI_MODE") == "false" | ||
|
|
||
| mock_s3 = mock.MagicMock() |
There was a problem hiding this comment.
I wanted to make sure if this is for s3 client. Is it a lambda client? (I might be missing something)
There was a problem hiding this comment.
My bad. It is the lambda client. Will fix.
Fix chalice deployment bug introduced in #270, closes #315
Category
slack_bolt.Appand/or its core componentsslack_bolt.async_app.AsyncAppand/or its core componentsslack_bolt.adapter/docsRequirements (place an
xin 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.
./scripts/install_all_and_run_tests.shafter making the changes.