-
Notifications
You must be signed in to change notification settings - Fork 11
ci: ensure JSON schemas are in sync #539
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
Conversation
| files_to_check=( | ||
| "packages/json_schemas/src/actor.schema.ts|packages/json_schemas/schemas/actor.schema.json" | ||
| # add more ts|json pairs here | ||
| ) | ||
|
|
||
| failed=0 | ||
|
|
||
| for pair in "${files_to_check[@]}"; do | ||
| ts_file="${pair%%|*}" | ||
| json_file="${pair##*|}" | ||
|
|
||
| if ! git diff --exit-code "$json_file" >/dev/null; then | ||
| echo "ERROR: $json_file is out of sync with $ts_file." | ||
| echo "Please put the schema changes to .ts file ($ts_file) and update the .json file ($json_file) by running the build script (npm run build)." | ||
| failed=1 | ||
| fi | ||
| done | ||
|
|
||
| if [ $failed -eq 1 ]; then | ||
| exit 1 | ||
| fi |
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 feels a bit overcomplicated, any reason you just don't diff the whole project? It will be fast enough, surely faster than running npm ci, building things or checking out the repo itself.
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.
I would literally just call git diff --exit-code, it will make the job fail if there are changes.
npm ci --no-audit
npm run build
git diff --exit-code
And this could be part of the regular test workflow, since there you need to build the project anyway, so why do it again separately. Project build should never produce any git changes, so it's a nice safeguard in general, not just for the JSON schema files.
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.
I believe this helps with DX. If someone who never saw this repository comes to update actor.json schema he would need to dig deeper to debug the failing workflow. With this he would just see exact instructions:
ERROR: packages/json_schemas/schemas/actor.schema.json is out of sync with packages/json_schemas/src/actor.schema.ts.
Please put the schema changes to .ts file (packages/json_schemas/src/actor.schema.ts) and update the .json file (packages/json_schemas/schemas/actor.schema.json) by running the build script (npm run build).
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.
Maybe we could do what @B4nan suggests, and just write out the message with exact instructions? It's pretty likely this is not going to change, and if it was supposed to change, we'd need to update the workflow anyway.
|
Nit: Maybe |

follow-up to #538
We need to ensure that for schemas that have
.tsfile used to generate.jsonfile:.tsfile not the.jsonfile.npm run buildto generate the.jsonfile and commits this generated file..tsand.jsonfiles are always in syncThis adds CI workflow that runs whenever this
.tsor.jsonfile changed, runs the build and check if.jsonfile changes or not.