feat: handle port not set and reverse proxy#540
feat: handle port not set and reverse proxy#540ramsayleung merged 3 commits intoramsayleung:masterfrom
Conversation
Summary of ChangesHello @ilteoood, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces significant improvements to how the application handles redirect URLs, particularly in containerized and reverse-proxied environments. It ensures that the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request enhances the get_socket_address function to support default ports for HTTP/HTTPS and to handle reverse proxy scenarios. The changes correctly implement the described logic by providing default ports when not specified and by falling back to a localhost address when the provided redirect URL host is not a loopback address. The test suite has been updated accordingly to cover these new cases.
I've found a couple of areas for improvement: one is a potential compilation error that needs to be addressed, and the other is a suggestion to improve code readability and maintainability.
|
Thanks for your contribution, the logic looks good to me, but it would be better if you could simplify the duplicated pattern of how to get default port for HTTP and HTTPS :) |
|
Thank you so much @ramsayleung ! |
|
Request accepted and processed, v0.15.2 is out :) |
Description
With this PR I would like to handle a couple of scenarios:
Motivation and Context
When the app is containerized + behind a reverse proxy, using the reverse proxy url makes the server to fail to start.
Dependencies
List any dependencies that are required for this change.
Type of change
Please delete options that are not relevant.
How has this been tested?
Changing the test scenario for the
get_socket_addressfunction.Is this change properly documented?
Please make sure you've properly documented the changes you're making.
Don't forget to add an entry to the CHANGELOG if necessary (new features, breaking changes, relevant internal improvements).