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

feat(LonaServer): Allow to set aiohttp client_max_size #224

Merged
merged 1 commit into from
Jul 4, 2022

Conversation

SmithChart
Copy link
Contributor

@SmithChart SmithChart commented May 26, 2022

With this change it is now possible to set the aiohttp
client_max_size during the creation of the Application. This
value controls the maximum size of a POST request in bytes.

This change basically moves the creation of the aiohttp Application
from run_server.py and app.py into the LonaServer.

Closes #222

Let me know what you think.

@fscherf fscherf requested a review from maratori May 30, 2022 18:32
@fscherf
Copy link
Member

fscherf commented May 30, 2022

@SmithChart: looks good to me. There seems to be no downside to create the app directly in the LonaServer constructor.
Where does the default for AIOHTTP_CLIENT_MAX_SIZE come from? Can you add it to the documentation? (The documentation does not build currently. I have a patch for it, you dont have to fix it yourself. Just add a sentence to the settings chapter)

@fscherf fscherf requested a review from laundmo May 30, 2022 18:38
@SmithChart
Copy link
Contributor Author

@SmithChart: looks good to me. There seems to be no downside to create the app directly in the LonaServer constructor. Where does the default for AIOHTTP_CLIENT_MAX_SIZE come from?

That is the AIOHTTP default value for this parameter.

Can you add it to the documentation?

Ack. I will update my PR with documentation.

@SmithChart
Copy link
Contributor Author

Can you add it to the documentation?

Ack. I will update my PR with documentation.

Documentation and commit message updated. Please Review.

@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2022

Codecov Report

Merging #224 (e9c3d9f) into master (d5fddd3) will increase coverage by 0.00%.
The diff coverage is 92.85%.

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

@@           Coverage Diff           @@
##           master     #224   +/-   ##
=======================================
  Coverage   62.25%   62.26%           
=======================================
  Files          67       67           
  Lines        4817     4818    +1     
  Branches      932      932           
=======================================
+ Hits         2999     3000    +1     
  Misses       1565     1565           
  Partials      253      253           
Impacted Files Coverage Δ
lona/command_line/run_server.py 26.41% <66.66%> (ø)
lona/app.py 58.28% <100.00%> (ø)
lona/default_settings.py 100.00% <100.00%> (ø)
lona/pytest.py 93.84% <100.00%> (-0.10%) ⬇️
lona/server.py 67.69% <100.00%> (+0.09%) ⬆️

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 d5fddd3...fe7edb6. Read the comment docs.

Copy link
Member

@fscherf fscherf 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 to me! Only some minor nitpicks. Besides them this looks ready to merge.

Can you remove the prefix "WIP: " when you are done?

lona/app.py Show resolved Hide resolved
:name: AIOHTTP_CLIENT_MAX_SIZE
:path: lona.default_settings.AIOHTTP_CLIENT_MAX_SIZE

.. note::
Copy link
Member

Choose a reason for hiding this comment

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

Can you please remove this message? The next version could be 1.11 as well.

Copy link
Contributor Author

@SmithChart SmithChart Jul 3, 2022

Choose a reason for hiding this comment

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

I hope I have understood correctly. I have now removed the ..note::section.
Please hit that resolve-button yourself if that's what you wanted :-)

@fscherf
Copy link
Member

fscherf commented Jun 27, 2022

@SmithChart: Whats the state of this PR?

With this change it is now possible to set the aiohttp
``client_max_size`` during the creation of the ``Application``. This
value controls the maximum size of a POST request in bytes.

This change basically moves the creation of the aiohttp ``Application``
from ``run_server.py`` and ``app.py`` into the ``LonaServer``.

Signed-off-by: Chris Fiege <chris@tinyhost.de>
@SmithChart SmithChart changed the title WIP: feat(LonaServer): Allow to set aiohttp client_max_size feat(LonaServer): Allow to set aiohttp client_max_size Jul 3, 2022
@SmithChart
Copy link
Contributor Author

Sorry for weeks of silence. First some intense weeks at work and afterwards some days vacation.
WIP-flag is removed. And I have also answered your nitpicks.

@fscherf
Copy link
Member

fscherf commented Jul 4, 2022

@SmithChart: No worries :) I just wanted to make sure you are not waiting for me

@fscherf
Copy link
Member

fscherf commented Jul 4, 2022

Looks good! Thanks for contributing :)

@fscherf fscherf merged commit 9f4fc1c into lona-web-org:master Jul 4, 2022
@SmithChart SmithChart deleted the client_max_size branch July 4, 2022 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Correct way to set aiohttp's client_max_size ?
3 participants