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.

Sign up for GitHub

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

Jump to bottom

Register encoding options when adding the service to the service collection #1575

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Register encoding options when adding the service to the service collection #1575

wants to merge 3 commits into from

Conversation

hugohlln
Copy link

@hugohlln hugohlln commented Aug 31, 2023 •
edited by ashishdhingra
Loading

Issue:: #1574

Description of changes:
Can now register content type for transformation by passing options to the HostingOptions object

@brignolij
Copy link

Good idea, Very usefull for webp images with api gateway.

@ashishdhingra ashishdhingra changed the title Can register encoding options when adding the service to the service … Can register encoding options when adding the service to the service collection. Sep 5, 2023
@dscpinheiro dscpinheiro changed the title Can register encoding options when adding the service to the service collection. Register encoding options when adding the service to the service collection Sep 5, 2023
@dscpinheiro dscpinheiro requested a review from normj September 5, 2023 17:24
normj
normj requested changes Sep 8, 2023
View reviewed changes
Copy link
Member

@normj normj left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I think the approach to solve the problem of configuring binary data is good. A few minor comments plus we also need some tests in the Amazon.Lambda.AspNetCoreServer.Test project.

Libraries/src/Amazon.Lambda.AspNetCoreServer.Hosting/HostingOptions.cs Show resolved Hide resolved
Libraries/src/Amazon.Lambda.AspNetCoreServer/Internal/EncodingOptions.cs Outdated Show resolved Hide resolved
Libraries/src/Amazon.Lambda.AspNetCoreServer/Internal/EncodingOptions.cs Outdated Show resolved Hide resolved
Libraries/src/Amazon.Lambda.AspNetCoreServer.Hosting/HostingOptions.cs Outdated Show resolved Hide resolved
@hugohlln
Copy link
Author

hugohlln commented Sep 8, 2023

@normj Thanks for your feedbacks

I just dit a new commit with following changes :

  • Add comments
  • Change the namespace of IEncodingOptions and EncodingOptions

For unit tests, what would be the best way to test the injection of IEncodingOptions ?

@ashishdhingra ashishdhingra added p2 This is a standard priority issue queued labels Sep 8, 2023
@ashishdhingra ashishdhingra requested a review from normj September 8, 2023 17:35
@CamileDahdah CamileDahdah self-requested a review September 27, 2023 17:11
@hugohlln
Copy link
Author

@CamileDahdah

I just moved the registration of IEncodingOptions to LambdaRuntimeSupportServer level and removed redundant code from each AbstractAspNetCoreFunction<TREQUEST, TRESPONSE> implementations (APIGatewayHttpApiV2MinimalApi, APIGatewayRestApiMinimalApi and ApplicationLoadBalancerMinimalApi)

@ashishdhingra ashishdhingra removed the request for review from CamileDahdah July 3, 2024 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CamileDahdah CamileDahdah CamileDahdah left review comments

@normj normj Awaiting requested review from normj

Assignees
No one assigned
Labels
bug This issue is a bug. module/aspnetcore-support p2 This is a standard priority issue queued
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hugohlln @brignolij @normj @CamileDahdah @ashishdhingra @hugohouillon