-
Notifications
You must be signed in to change notification settings - Fork 1.1k
print_banner: print actual port in case the port is dynamic #1450
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
base: master
Are you sure you want to change the base?
Conversation
|
|
||
| print_banner(self._app) | ||
|
|
||
| self.assertIn(f'INFO:flower.command:Visit me at http://0.0.0.0:{port}', cm.output) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the unit test for the dynamic port number showing up in the banner.
3b07ff0 to
78ddaa6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where the banner displayed incorrect port information (showing port 0) when Flower is started with dynamic port allocation (--port=0). The solution refactors the print_banner function to receive the Flower app instance instead of the Celery app, allowing it to access the actual bound port.
Key changes:
- Decoupled server startup from event loop execution to determine the actual port before printing the banner
- Modified
print_bannerto accept Flower app instance and retrieve dynamic port information - Refactored test base class from
AsyncHTTPTestCasetoAsyncTestCasefor better control over HTTP server lifecycle
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| flower/command.py | Updated to separate server startup from event loop and pass Flower app to print_banner |
| flower/app.py | Added methods for server lifecycle management and URL generation with dynamic port support |
| tests/unit/init.py | Refactored base test class to manage HTTP server directly instead of using Tornado's built-in server |
| tests/unit/test_command.py | Updated tests to use new banner function signature and added dynamic port test |
| tests/unit/views/*.py | Updated to use self._app instead of self.app following test base class changes |
| tests/unit/api/*.py | Updated to use self._app instead of self.app following test base class changes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
78ddaa6 to
5be94b5
Compare
5be94b5 to
b0ca57e
Compare
| url = flower_app.get_url() | ||
| logger.info("Visit me at %s", url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the fix. We get the URL from the Flower app. Before we got it from the Celery app before the server started.
Closes #1449
When starting flower with
--port=0the port is dynamically assigned butprint_bannershowsVisit me at http://0.0.0.0:0.print_bannerinstead of the celery app.AsyncHTTPTestCasebase class needs to be in charge of creating the HTTP server. Before it wastornado.testing.AsyncHTTPTestCasedoing this. Now we derive fromtornado.testing.AsyncTestCaseand letFlowerhandle the HTTP server. This happens in production anyway so now we cover that as well. In addition this makes the usage in other tests much cleaner.