-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Fix the underlying component schema to use check instead of checker #19910
Conversation
"SimpleRetriever": { | ||
"allOf": [ | ||
{ | ||
"$ref": "#/definitions/Retriever" | ||
}, | ||
{ | ||
"type": "object", | ||
"required": ["requester", "record_selector"], | ||
"required": ["requester", "record_selector", "config"], |
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.
Do you know why this PR is adding config
as a required field to several of these objects? I think ideally this would not be required, since we are not planning to set this in the connector builder
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 because we haven't regenerated the schema in at least a few weeks and there were some component classes that had the config object added as a field. So that gets reflected in the auto-generated schema. I did some spot checks and this is accurate to the current fields of the SimpleRetriever class.
Sadly this is just another case of the downsides of the autogenerated schema since we're coupled to the underlying implementation which passes things like config under the hood
/publish-cdk dry-run=false
|
What
The manifest component schema that is generated dynamically based on the low-code data model and used to validate manifests expects
checker
but it should really be validating thatcheck
is present.How
In the class ConcreteDeclarativeSource, the dataclass field is called
checker
which then formats the manifest by that name. This is causing an issue for the form based connector builder UI. For some reason in a manifest we take the contents ofcheck
and place them underchecker
. We should just do away with this entirely and format everything under checker. This PR does that and also regenerates the static schema as well.Given that all manifests were already defining the
check
, this isn't a breaking change, we're just doing away with remapping it to checker which seems unnecessary.Recommended reading order
manifest_declarative_source.py