Skip to content

Conversation

@tisnik
Copy link
Contributor

@tisnik tisnik commented Dec 10, 2025

Description

LCORE-1092: Better docstring for Uvicorn runner

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement

Tools used to create PR

Identify any AI code assistants used in this PR (for transparency and review context)

  • Assisted-by: CodeRabbitAI
  • Generated by: N/A

Related Tickets & Documents

  • Related Issue #LCORE-1092

Summary by CodeRabbit

  • Documentation
    • Expanded documentation for TLS configuration options (certificate/key paths and optional key password) and how those settings are passed into the server runtime; clarifies usage without changing runtime behavior.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 10, 2025

Walkthrough

The start_uvicorn function's docstring was expanded to describe how it uses ServiceConfiguration fields — including tls_key_path, tls_certificate_path, and tls_key_password — and that these are forwarded to uvicorn.run. No code, parameters, return types, or control flow were changed.

Changes

Cohort / File(s) Summary
Documentation Update
src/runners/uvicorn.py
Expanded multi-line docstring for start_uvicorn describing usage of ServiceConfiguration fields (including TLS fields) and their forwarding to uvicorn.run. No functional changes.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

  • Focus review on docstring clarity and accuracy in src/runners/uvicorn.py.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: improving documentation for the Uvicorn runner function.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e9e2991 and 8a91fb5.

📒 Files selected for processing (1)
  • src/runners/uvicorn.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/runners/uvicorn.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: build-pr
  • GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
  • GitHub Check: E2E: library mode / ci
  • GitHub Check: E2E: library mode / azure
  • GitHub Check: E2E: server mode / ci
  • GitHub Check: E2E: server mode / azure

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 668d09c and e9e2991.

📒 Files selected for processing (1)
  • src/runners/uvicorn.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.py: Use absolute imports for internal modules in LCS project (e.g., from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Use logger = logging.getLogger(__name__) pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) and Optional[Type] or Type | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Use async def for I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes: Configuration for config classes, Error/Exception for exceptions, Resolver for strategy patterns, Interface for abstract base classes
Abstract classes must use ABC with @abstractmethod decorators
Include complete type annotations for all class attributes in Python classes
Use import logging and module logger pattern with standard log levels: debug, info, warning, error

Files:

  • src/runners/uvicorn.py
🪛 GitHub Actions: Black
src/runners/uvicorn.py

[error] 1-1: Black formatting check failed. 1 file would be reformatted: /home/runner/work/lightspeed-stack/lightspeed-stack/src/runners/uvicorn.py. Run 'black' to format. Command: 'uv tool run black --check .' exited with code 1.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: build-pr
  • GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
  • GitHub Check: E2E: server mode / azure
  • GitHub Check: E2E: library mode / azure
  • GitHub Check: E2E: server mode / ci
  • GitHub Check: E2E: library mode / ci

Comment on lines 13 to 21
"""
Start the Uvicorn server using the provided service configuration.
Parameters:
configuration (ServiceConfiguration): Configuration providing host,
port, workers, and `tls_config` (including `tls_key_path`,
`tls_certificate_path`, and `tls_key_password`). TLS fields may be None
and will be forwarded to uvicorn.run as provided.
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Fix Black formatting failure and clarify tls_key_password handling.

The pipeline reports that Black would reformat this file. Additionally, the docstring states TLS fields "will be forwarded to uvicorn.run as provided," but line 36 shows that tls_key_password is converted from None to an empty string "" rather than being passed as-is.

Run Black to fix the formatting:

#!/bin/bash
# Fix Black formatting
black src/runners/uvicorn.py

Consider this diff to clarify the tls_key_password behavior:

     """
     Start the Uvicorn server using the provided service configuration.
     
     Parameters:
         configuration (ServiceConfiguration): Configuration providing host,
         port, workers, and `tls_config` (including `tls_key_path`,
-        `tls_certificate_path`, and `tls_key_password`). TLS fields may be None
-        and will be forwarded to uvicorn.run as provided.
+        `tls_certificate_path`, and `tls_key_password`). TLS key and certificate
+        paths may be None. The tls_key_password is converted to an empty string
+        if None.
     """
🤖 Prompt for AI Agents
In src/runners/uvicorn.py around lines 13 to 21, run Black to fix formatting
(e.g., black src/runners/uvicorn.py) and update the docstring to accurately
describe TLS password handling: state that tls_key_password is normalized from
None to an empty string ("") before being forwarded to uvicorn.run (or adjust
the code if you prefer to pass None through), and ensure the docstring wording
and punctuation match Black's formatting rules so the file passes the formatter.

@tisnik tisnik force-pushed the lcore-1092-better-docstring-for-uvicorn-runner branch from e9e2991 to 8a91fb5 Compare December 10, 2025 07:43
@tisnik tisnik merged commit 008a010 into lightspeed-core:main Dec 10, 2025
21 of 25 checks passed
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.

1 participant