-
Notifications
You must be signed in to change notification settings - Fork 6
Test #187
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
Test #187
Conversation
Reviewer's GuideThis PR restructures the application’s configuration by introducing a new “custom” section in config.yaml backed by a Pydantic CustomConfig model, updates code to reference the nested custom settings (controllers, TES, logging, middlewares, serviceInfo), adjusts app initialization to use the new model, and adds a small documentation link. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @Nidhi091999 - I've reviewed your changes - here's some feedback:
- The Celery config block was left at the root instead of under custom.controllers.celery—move it under the new custom.controllers hierarchy to match your CustomConfig model.
- YAML uses 'serviceInfo' but the Pydantic model defines 'service_info'; either rename the key or add an alias in the model so they align.
- You're indexing Pydantic models like dicts (e.g. custom.storeLogs["execution_trace"]); switch to attribute access (custom.storeLogs.execution_trace) for type safety and clarity.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
tes: Tes = Tes() | ||
storeLogs: StoreLogs = StoreLogs() | ||
middlewares: Middlewares = Middlewares() | ||
service_info: TesServiceInfo |
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.
issue (bug_risk): YAML key 'serviceInfo' does not match Pydantic field 'service_info'
Add Field(..., alias='serviceInfo')
to service_info
or rename it to serviceInfo
so the YAML key aligns and loads correctly.
@@ -86,7 +86,7 @@ def create_task( # pylint: disable=too-many-statements,too-many-branches | |||
# apply middlewares | |||
mw_handler = MiddlewareHandler() | |||
mw_handler.set_middlewares( | |||
paths=current_app.config.foca.middlewares # type: ignore | |||
paths=current_app.config.foca.custom.middlewares # type: ignore |
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.
issue (bug_risk): Passing Pydantic model instead of raw list to middleware handler
Pass middlewares.__root__
or update the model to expose the list as a normal field, so set_middlewares
receives an actual list.
checks
Summary by Sourcery
Introduce a custom configuration model and namespace existing settings under a dedicated 'custom' key, updating code references and documentation accordingly.
New Features:
custom_config_model
parameter in app initialization.Enhancements:
custom
section in config.yaml.foca.config.custom
instead of the root config fields.Documentation: