-
Notifications
You must be signed in to change notification settings - Fork 12
Fix gunicorn logging in docker #390
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: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain why this was put in its own file/made into a function?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Gunicorn command now requires deployment-specific logic (Docker vs non-Docker). Extracting it into build_gunicorn_command() centralizes command construction, makes the behavior testable, avoids duplicating Docker logging logic elsewhere, and keeps start() focused on process startup rather than command assembly.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the non-docker usecase is largely deprecated in collectoss, and just hasn't really been removed yet |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| # SPDX-License-Identifier: MIT | ||
| """ | ||
| Helpers for starting Gunicorn from CollectOSS CLI commands. | ||
| """ | ||
| from __future__ import annotations | ||
|
|
||
| import os | ||
| from typing import Optional | ||
|
|
||
| from collectoss.application.environment import SystemEnv | ||
|
|
||
|
|
||
| def is_docker_deploy() -> bool: | ||
| return SystemEnv.get_bool("COLLECTOSS_DOCKER_DEPLOY", False) | ||
|
|
||
|
|
||
| def build_gunicorn_command( | ||
| gunicorn_config: str, | ||
| host: str, | ||
| port: str, | ||
| app: str = "collectoss.api.server:app", | ||
| log_file: Optional[str] = None, | ||
| docker_deploy: Optional[bool] = None, | ||
| ) -> list[str]: | ||
| """ | ||
| Build a Gunicorn command while preserving Docker stderr logging. | ||
|
|
||
| In Docker, Gunicorn's config sends error logs to "-" so failures are visible | ||
| through container logs. Passing --log-file would override that setting. | ||
| """ | ||
| command = [ | ||
| "gunicorn", | ||
| "-c", | ||
| gunicorn_config, | ||
| "-b", | ||
| f"{host}:{port}", | ||
| app, | ||
| ] | ||
|
|
||
| if docker_deploy is None: | ||
| docker_deploy = is_docker_deploy() | ||
|
|
||
| if log_file and not docker_deploy: | ||
| command.extend(["--log-file", os.fspath(log_file)]) | ||
|
|
||
| return command |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| # SPDX-License-Identifier: MIT | ||
| """Tests for Gunicorn CLI command construction.""" | ||
|
|
||
| from collectoss.application.cli._gunicorn import build_gunicorn_command | ||
|
|
||
|
|
||
| def test_gunicorn_command_uses_log_file_outside_docker(): | ||
| command = build_gunicorn_command( | ||
| "/collectoss/collectoss/api/gunicorn_conf.py", | ||
| "0.0.0.0", | ||
| "5000", | ||
| log_file="/collectoss/logs/gunicorn.log", | ||
| docker_deploy=False, | ||
| ) | ||
|
|
||
| assert command == [ | ||
| "gunicorn", | ||
| "-c", | ||
| "/collectoss/collectoss/api/gunicorn_conf.py", | ||
| "-b", | ||
| "0.0.0.0:5000", | ||
| "collectoss.api.server:app", | ||
| "--log-file", | ||
| "/collectoss/logs/gunicorn.log", | ||
| ] | ||
|
|
||
|
|
||
| def test_gunicorn_command_does_not_override_docker_errorlog(): | ||
| command = build_gunicorn_command( | ||
| "/collectoss/collectoss/api/gunicorn_conf.py", | ||
| "0.0.0.0", | ||
| "5000", | ||
| log_file="/collectoss/logs/gunicorn.log", | ||
| docker_deploy=True, | ||
| ) | ||
|
|
||
| assert "--log-file" not in command | ||
| assert command == [ | ||
| "gunicorn", | ||
| "-c", | ||
| "/collectoss/collectoss/api/gunicorn_conf.py", | ||
| "-b", | ||
| "0.0.0.0:5000", | ||
| "collectoss.api.server:app", | ||
| ] |
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 a change that was made in another PR already. can you rebase your PR on top of current main?