Commit the API schema to the repository #5865
Conversation
|
/check |
|
❌ Some checks failed |
SpecLad
force-pushed
the
schema-in-repo
branch
6 times, most recently
from
c6af455 to
796d369
Compare
SpecLad
requested review from
zhiltsov-max
and removed request for
nmanovic and
azhavoro
| @@ -19,55 +19,29 @@ the repository. To get the full package, one need to generate missing package fi | |||
|
|
|||
| ## How to generate package code | |||
|
|
|||
| 1. Obtain the server API schema | |||
| 1. Install generator dependencies: | |||
There was a problem hiding this comment.
Where can I see steps on how to generate the schema manually (except .github/*.yml files)?
There was a problem hiding this comment.
I broke the schema. Should I generate a new one?
There was a problem hiding this comment.
In this case, the CI check prints the command you need to run. There's also a VS code launch configuration that does it.
There was a problem hiding this comment.
In this case, the CI check prints the command you need to run.
There is one problem: The schema may not be generated correctly If we have an outdated image.
There's also a VS code launch configuration that does it.
In this case, for some reason some fields in the schema lose their min/max limitations.
There was a problem hiding this comment.
Agree, I've also faced this difference in the generated schema.
There was a problem hiding this comment.
I have found the cause for this discrepancy; it's because in the Docker image, the production configuration is selected, which uses a different database. I'll submit a fix to the VS code configuration.
Instead of generating the schema during the SDK build, store it in the repository. Add a CI check that verifies that the schema matches the server. This has the following benefits: * It becomes easy to see if and how a pull request affects the API (syntactically, at least). Since the schema is generated dynamically by drf-spectacular, it's not always obvious what effects a code change will have on the declared API. By requiring PRs to explicitly change the schema, we enable such changes to be reviewed directly. * It removes one step from the SDK generation procedure, making it easier for people to contribute. The schema generation step was, IMO, the most annoying (because you have to build the server) and the most error-prone (because it's easy to use the wrong version of the server). There's also a drawback: there's now one more step to change the API, updating the schema. I think this is acceptable, because if you changed the API, then to test your modifications you probably started the new version of the server, so you can just get the schema from there. To make it easier, make the CI provide the expected version of the schema, the diff from the current version, and the command to regenerate the schema locally.
Instead of generating the schema during the SDK build, store it in the repository. Add a CI check that verifies that the schema matches the server.
Motivation and context
This has the following benefits:
It becomes easy to see if and how a pull request affects the API (syntactically, at least). Since the schema is generated dynamically by drf-spectacular, it's not always obvious what effects a code change will have on the declared API. By requiring PRs to explicitly change the schema, we enable such changes to be reviewed directly.
It removes one step from the SDK generation procedure, making it easier for people to contribute. The schema generation step was, IMO, the most annoying (because you have to build the server) and the most error-prone (because it's easy to use the wrong version of the server).
There's also a drawback: there's now one more step to change the API, updating the schema. I think this is acceptable, because if you changed the API, then to test your modifications you probably started the new version of the server, so you can just get the schema from there. To make it easier, make the CI provide the expected version of the schema, the diff from the current version, and the command to regenerate the schema locally.
How has this been tested?
Mainly by running the CI. I added artificial problems to the PR to make sure the schema verification works, and also to make sure that the unit test failure reporting still works after my changes.
Checklist
developbranch[ ] I have added a description of my changes into the CHANGELOG fileI don't think this is changelog-worthy, because all changes are developer-facing.[ ] I have linked related issues (see GitHub docs)[ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.