Skip to content

Allow lazy function invocation to target version/alias - #679

Merged
seratch merged 2 commits into
slackapi:mainfrom
angrychimp:main
Jul 12, 2022
Merged

seratch merged 2 commits into
slackapi:mainfrom
angrychimp:main

Conversation

@angrychimp

@angrychimp angrychimp commented Jul 11, 2022 •

Copy link
Copy Markdown
Contributor

Currently lazy responses are only targeting the $LATEST version of a given AWS function. This prevents meaningful A/B testing if you're using function versions/aliases to compare code changes. Adjusting the invoke method to use the function ARN from the current execution allows downstream code parity.

This change adds a variable (invoked_function_arn) to the request context, then uses that value when invoking the sub-request.

Category

  • 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 Jul 11, 2022 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@seratch seratch self-assigned this Jul 11, 2022
@seratch seratch added enhancement New feature or request area:adapter labels Jul 11, 2022
@seratch seratch added this to the 1.14.1 milestone Jul 11, 2022

@seratch seratch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for improving this! LGTM

event["method"] = "NONE"
invocation = self.lambda_client.invoke(
FunctionName=request.context["aws_lambda_function_name"],
FunctionName=request.context["aws_lambda_invoked_function_arn"],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should work. The lambda invocation API accepts any of function name, function ARN, and partial ARN. https://docs.aws.amazon.com/cli/latest/reference/lambda/invoke.html#options

@seratch

seratch commented Jul 11, 2022

Copy link
Copy Markdown
Contributor

@angrychimp The change looks great to me already but this change requires a small update in the unit test code. Could you update the test code to be consistent with your code change?

@angrychimp

Copy link
Copy Markdown
Contributor Author

Not sure how I missed that, but thanks for pointing it out. I've added the updated test script to the PR. I'm getting a seg-fault running the "run_tests" script which I believe is just an issue with my environment, but while I figure that out let me know if it looks like there's anything else I'm missing.

@angrychimp The change looks great to me already but this change requires a small update in the unit test code. Could you update the test code to be consistent with your code change?

@codecov

codecov Bot commented Jul 11, 2022

Copy link
Copy Markdown

Codecov Report

Merging #679 (7629bd4) into main (208992c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 7629bd4 differs from pull request most recent head e756010. Consider uploading reports for the commit e756010 to get more accurate results

@@           Coverage Diff           @@
##             main     #679   +/-   ##
=======================================
  Coverage   92.05%   92.05%           
=======================================
  Files         172      172           
  Lines        5864     5865    +1     
=======================================
+ Hits         5398     5399    +1     
  Misses        466      466           
Impacted Files Coverage Δ
...ck_bolt/adapter/aws_lambda/lazy_listener_runner.py 55.00% <ø> (ø)
slack_bolt/adapter/aws_lambda/handler.py 95.89% <100.00%> (+0.05%) ⬆️

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 208992c...e756010. Read the comment docs.

@seratch

seratch commented Jul 12, 2022

Copy link
Copy Markdown
Contributor

@angrychimp

I'm getting a seg-fault running the "run_tests" script which I believe is just an issue with my environment

We are aware of this potential error that can rarely happen but this is it not related to your changes. Also, retrying the test execution should succeed.

The CI builds are now successful! Thanks a lot for your contribution 🎉

@seratch
seratch merged commit 421c0d0 into slackapi:main Jul 12, 2022
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