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

AWS HTTP API: Ensure function timeout setting is respected #7420

Merged
merged 1 commit into from Mar 4, 2020

Conversation

medikoo
Copy link
Contributor

@medikoo medikoo commented Mar 3, 2020

Additionally deprecate (by not documenting) a timeout setting on HTTP API endpoint.
As HTTP API endpoints can only be configured through functions, it seems logical to treat function timeout setting as the only source of truth.

Should be considered as bug fix.

@Nemo64
Copy link

Nemo64 commented Mar 3, 2020

deprecation by not documenting seems really dangerous. How would anyone notice who is already using that option?

Either don't deprecate, leave the documentation and say that the default value is based on the lambda function timeout or print a warning so that everyone using it knows that this option may be dropped in the next mayor release.

I would tend to leave the option.

Additionally deprecate (by not documenting) a `timeout` setting on HTT API endpoint.
As HTTP API endpoints can only be configured through functions, it seems logical to treat function timeout setting as the only source of truth.
@medikoo medikoo force-pushed the 0303-http-api-adjust-timeout branch from 267f25f to 306710a Compare March 3, 2020 21:43
@medikoo
Copy link
Contributor Author

medikoo commented Mar 3, 2020

deprecation by not documenting seems really dangerous. How would anyone notice who is already using that option?

Good point. I've added deprecation log.

@codecov-io
Copy link

Codecov Report

Merging #7420 into master will increase coverage by <.01%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7420      +/-   ##
==========================================
+ Coverage   87.99%   87.99%   +<.01%     
==========================================
  Files         240      240              
  Lines        9052     9063      +11     
==========================================
+ Hits         7965     7975      +10     
- Misses       1087     1088       +1
Impacted Files Coverage Δ
...lugins/aws/package/compile/events/httpApi/index.js 88.6% <90.9%> (+0.17%) ⬆️

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 ba84179...306710a. Read the comment docs.

Copy link
Member

@eahefnawy eahefnawy left a comment

Choose a reason for hiding this comment

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

Looks good @medikoo 👌

Just to be sure though, users are still able to configure HTTP APIs timeout, it's just that we compare that to the function timeout , correct?

@medikoo
Copy link
Contributor Author

medikoo commented Mar 4, 2020

Just to be sure though, users are still able to configure HTTP APIs timeout, it's just that we compare that to the function timeout , correct?

Yes, I left ability to configure timeout on httpApi event but it's now deprecated to do so. Users should just follow settings on lambda (I think there's no valid use case to provide two different timeouts for lambda and HTTP endpoint)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants