-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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: update permalink schema #24970
fix: update permalink schema #24970
Conversation
@eschutho Should we also update "ExplorePermalinkPostSchema": {
"properties": {
"formData": {
"description": "Chart form data",
"type": "object"
},
"urlParams": {
"description": "URL Parameters",
"items": {
"description": "URL Parameter key-value pair",
"nullable": true
},
"nullable": true,
"type": "array"
}
},
"required": ["formData"],
"type": "object"
}, |
Yeah, good catch. Not sure if removing it is the right way to go, so lmk what you think. 🙏 |
@@ -5454,25 +5454,6 @@ | |||
}, | |||
"type": "object" | |||
}, | |||
"ExplorePermalinkPostSchema": { |
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.
should you just rename this to ExplorePermalinkStateSchema
?
maybe just use https://github.com/preset-io/superset/blob/master/superset/cli/update.py#L71 to update?
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.
Nice, it looks like the cli command hasn't been run in a while. I've committed the output, if we think it's worth cleaning the whole file up in the process.
400e63f
to
5b41c3e
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 for fixing this @eschutho - I've indeed been looking into why our current OAS test isn't picking up this error, and it appears that regression is still there in the latest version of the validator library. I'll keep an eye out for a potential fix, and if one doesn't show up soon, I'll try to make a simple repro case and share it with the library devs.
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.
LGTM. Thank you @eschutho!
Thanks all! |
(cherry picked from commit bc1c5c2)
SUMMARY
Small fix for #24586 to update the schema to point to the new one. The PostSchema no longer exists, so it was failing a test in the 2.1.1 release.
TESTING INSTRUCTIONS
The test in master isn't failing with the Post schema, likely because of some regression with openapi-spec-validator==0.5.6 that @villebro is looking into
ADDITIONAL INFORMATION