-
Notifications
You must be signed in to change notification settings - Fork 6
feat: models for custom config validation #186
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: dev
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request introduces custom configuration validation using Pydantic models. A new file No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Assessment against linked issues
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 and found some issues that need to be addressed.
Blocking issues:
- Hardcoded boolean value for execution trace logs. (link)
Overall Comments:
- The Pydantic models in
config_models.py
appear to contain YAML snippets instead of valid field definitions and nested models. - Consider using snake_case (e.g.,
store_logs
) for attribute names within theCustomConfig
model for consistency with Python conventions.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🔴 Security: 1 blocking issue
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 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.
0cd8875
to
0e2b663
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.
Thanks a lot, @Nidhi091999 - as far as I can tell, the models look fine now. However, there are 3 issues remaining:
- The app configuration now needs to be updated to include a
custom
section, which is required to activate the validation. The sections defining the values for custom config params, likecontrollers
,serviceInfo
and so on (see https://github.com/elixir-cloud-aai/proTES/blob/f83c3e3cd10eef83829c9f1359830a0d99f4299a/pro_tes/config.yaml#L109C1-L149C84 for all of them) need to children of sectioncustom
. - Given that the new
custom
section changes the paths under which these params can be found when used in the app, all code referencing any params in thecustom
section need to be updated accordingly, e.g., fromcurrent_app.config.foca.tes["service_list"]
tocurrent_app.config.foca.custom.tes["service_list"]
- The integration tests are currently failing. I am not sure why that is, but possibly it is because currently there is no custom section in
pro_tes/config.yaml
; so possibly, if you fix point 1 above (which in turns requires addressing point 2), this problem might resolve itself. If not, please try to add some debug statements to see why the tests are failing with the new models.
Resolves #164
Summary by Sourcery
Implement custom configuration validation using Pydantic models.
New Features:
Controllers
,Tes
,StoreLogs
,Middlewares
,ServiceInfo
).