-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add auto-generated Configuration schema from Pydantic model #16518
base: dev
Are you sure you want to change the base?
Add auto-generated Configuration schema from Pydantic model #16518
Conversation
This is amazing, I love it. How about we turn this around and make the pydantic model the source of truth from which we generate the other stuff ... it should be much more expressive than what we have currently, and the model_validators would let us for instance easily flag stuff that can't be combined. And we can easily define what can and can't be exposed through the API inline. |
Thanks so much @mvdbeek! That is exactly what I needed to hear 😄 👍 |
609401b
to
7efd287
Compare
a18cb33
to
329989c
Compare
91d7206
to
7a20cd0
Compare
From the configuration schema.
And invoke `config-api-schema` when building the config options and updating the client API schema.
This model is not yet properly generated as it exposes everything.
- Fix some encoding issues - Use Annotated for fields
This reverts commit 554ddee.
See test/integration/test_data_manager.py::TestDataManagerIntegration::test_data_manager_installation_table_reload
The documentation says that the option is called `extended_celery` but the tests were using `celery_extended`. The logic will accept both options, but the tests should be consistent with the documentation.
The validation logic needs to live in the manager class because otherwise the configuration package will need to depend on the schema, pydantic, etc.
When validating the config file, is a list of objects but when serialized to the API is a dictionary.
The option `use_remote_user` was used an declared as bool in the codebase, but it was assigned the value of `single_user` in some cases and that option can be either a bool or an email address (str) causing the type validation to fail.
This is somewhat controversial, but otherwise the generated schema will have weird spacing and unwanted line breaks in all descriptions.
c9a6794
to
b27ee9a
Compare
Discussed this in the backend channel, we're all on board with having the pydantic models as the source of truth, and that if necessary, we can replace the tooling built on pykwalify. |
Follow up to #16514
I was manually creating the model when I realized we already have the
config_schema.yml
. It would be much easier to maintain if we could update the model whenever we introduce a new config parameter. As wisely suggested by Marius, I will approach this by first generating the initial basic Pydantic Model from the config schema, and then, the idea is to use the model as the single source of truth and generate the schema from it.TODO
config_schema.yml
views
of the Configuration model, initially:ComputedGalaxyConfig
with admin-only options.tool_shed_urls
directly from the API #16561 in modelby_host
dynamic config options into account Allow various configuration parameters to be set per host. #12328path_resolves_to
optionsreloadable
optionsuse_remote_user
getting a string (email) instead of a boolean as a value assigned from thesingle_user
option in some cases. I decided to fix it by casting to the bool value which is more explicit c9a6794config_schema.yml
from theFull
modelHow to test the changes?
(Select all options that apply)
License