Repository navigation
ci: fail when the published OpenAPI spec is stale - #43841
Conversation
|
Bito Automatic Review Skipped - Files Excluded |
c8fa99d to
1fd71eb
Compare
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| # paths that would 404 for everyone who has not enabled them. | ||
| SUPERSET__SQLALCHEMY_DATABASE_URI: "sqlite:///:memory:" | ||
| FLASK_APP: "superset.app:create_app()" | ||
| run: superset update-api-docs |
There was a problem hiding this comment.
This new required check is already failing because regenerating the spec changes openapi.json (110 additions, 46 deletions), so the PR cannot merge as written. Should the regenerated artifact be included with this guard?
|
Yes, the regenerated artifact should be included in the PR. The current workflow is designed to enforce that the checked-in .github/workflows/check-openapi-spec-drift.yml |
| # paths that would 404 for everyone who has not enabled them. | ||
| SUPERSET__SQLALCHEMY_DATABASE_URI: "sqlite:///:memory:" | ||
| FLASK_APP: "superset.app:create_app()" | ||
| run: superset update-api-docs |
There was a problem hiding this comment.
If a registration regression leaves the app with no v1 BaseApi, superset update-api-docs only prints API version not found and exits successfully without rewriting the file. The subsequent diff check then passes against the old committed spec, so this guard misses precisely that broken-generation state. Could this step fail when no spec was generated (or assert that the output was produced) before checking the diff?
There was a problem hiding this comment.
Good catch — fixed in the update-api-docs CLI rather than the workflow, so every caller fails not just this guard.
The spec is generated from the running app but committed by hand, so it drifts silently — the last genuine regeneration was apache#33378, sixteen months before the one in apache#43788. Regenerate on every PR and fail on a dirty diff. The job installs requirements/base.txt and passes no config file, because both inputs change the output: apispec 6.10.0 renders marshmallow 4's unknown=RAISE as "additionalProperties": false where the pinned 6.6.1 does not, and a config with feature flags enabled documents paths that 404 by default.
The command printed "API version not found" to stderr and exited zero, leaving the committed spec untouched. Any caller diffing the result — including the drift guard added here — read that as an up-to-date spec.
197e9a6 to
9cd960a
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43841 +/- ##
=======================================
Coverage 79.46% 79.46%
=======================================
Files 2895 2895
Lines 168167 168167
Branches 38995 38995
=======================================
+ Hits 133627 133630 +3
+ Misses 32041 32036 -5
- Partials 2499 2501 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
villebro
left a comment
There was a problem hiding this comment.
LGTM, this is awesome! Waiting for CI to finish
|
Bito Automatic Review Skipped – PR Already Merged |
SUMMARY
docs/static/resources/openapi.jsonis generated from the running app but committed by hand, so it drifts silently. The last genuine regeneration before #43788 was #33378, sixteen months earlier. This regenerates the spec on every PR and fails on a dirty diff.Two generation inputs are pinned deliberately, because getting either wrong was a real failure while preparing #43788. The job installs
requirements/base.txt: apispec 6.10.0 renders marshmallow 4'sunknown=RAISEas"additionalProperties": falsewhere the pinned 6.6.1 does not, so an unpinned guard would fail every PR. And it passes no config file, so generation reflects the routes a default deployment registers — a config with feature flags enabled documents paths that 404 for everyone who hasn't enabled them.No
paths:filter, matchingenforce-single-migration-head: a required check that never runs on a PR blocks it forever. The job is ~10s.TESTING INSTRUCTIONS
Passes on unmodified
master— the committed spec reproduces byte-identically on the pinned requirements. To see it fail, add a field to any marshmallow schema without regenerating:Adding one schema renumbers apispec's auto-generated suffixes (
User1,Subject1), so diffs are routinely larger than the change.ADDITIONAL INFORMATION