-
Notifications
You must be signed in to change notification settings - Fork 75
feat: Add storage layout check script to ci #472
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
Simple script that errors if developer makes changes that would change the storage layout but hasn't committed new storage layout file
a168e0f to
54b2ddd
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.
I can put up a test PR that changes the diff to test that this fails. Otherwise LGTM
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.
Looks great! Just minor nits and questions about comments
| strategy: | ||
| matrix: | ||
| node: [16] |
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.
Is there a need to use matrix? Looks like it is only one param to use (not sure if you guys are planning to run with other nodes too). By the way node 16 is no longer maintained and considered deprecated https://nodejs.org/en/blog/announcements/nodejs16-eol
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.
yeah we need to move off of this but our stack currently can't automatically upgrade. @james-a-morris is working on this
Co-authored-by: Matt Rice <matthewcrice32@gmail.com>
Simple script that errors if developer makes changes that would change the storage layout but hasn't committed new storage layout file