-
-
Notifications
You must be signed in to change notification settings - Fork 191
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
feat: use an external json schema for botconfig #1894
Conversation
Need to use orjson because pydantic.Url is not json dumpable
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 great. I'm fine producing the schema right in this repo and not having to use a gist. I'm guessing we'll have to merge once before that works?
Edit: I thought this was cf-scripts. Now this PR makes more sense to me. 🤦♂️
We'll move the Pydantic model to regro/cf-scripts and dump the schema to that repo too. Then we can use the URL for that file here. |
Some thoughts about this:
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
requires regro/cf-scripts#3789 to pass |
So, do I understand correctly that you decided to no longer support validating |
No we did not. We moved the serialized json schema and pydantic model to the bot code base. The linter uses the json. However, I'd accept a PR that adds an optional, non-default feature that imports the bot's model and makes a pydantic model at the python level that combines the smithy+bot ones. People can then import that model via python to do validation if they like. The approach here isn't exclusive, but rather to have optional features for those who want them. We cannot use the pydantic python code by default since it would create a circular dependency between the bots code base and smithy. |
Unfortunately, dynamically generated pydantic models do not allow static type checking, which is a large part of the benefit I see in pydantic. Also, I feel it's problematic that we'd then have two different models describing the same thing.
We could avoid this by moving only the bot's pydantic model to a separate conda package. Both conda-smithy and the bot could depend on that package. WDYT about that? |
I'm against extra packages here for the same reasons as discussed for smithy. It sounds like we can support some of the functionality but not all of it. As I understand it, this model is mostly used to check yaml/json files, not python code. So I think we've reached a decent compromise. |
It's been almost a year and no one is using the pydantic model AFAIK (other than dumping the json schema and validating through that) |
cf-scripts uses it here: https://github.com/regro/cf-scripts/blob/main/conda_forge_tick/models/node_attributes.py#L4 |
Right and the model is now in cf-scripts so this will still work fine. |
As it says above...
bot schema was moved over here: regro/cf-scripts#3789
cc @ytausch @beckermr
closes #1893