-
Notifications
You must be signed in to change notification settings - Fork 32
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
Add a step to the node CI test to confirm built scripts are included in the change #374
Conversation
The screenshots indicate that it's trying to protect against two cases, but the error message only points towards one? |
.github/workflows/node.js.yml
Outdated
echo "$REPOSTATUS"; | ||
exit 1; | ||
else | ||
echo The repo file system is clean.; |
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.
While true, it isn't indicative of what was actually being checked - that built files are present or not. Right now, those unfamiliar may not be able to join the dots between whether the file system is clean (is that good or bad?) and the real potential problem.
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've added a clarification message regarding this.
Description
After CI runs
npm run build
, flag the branch a failure if any uncommitted changes are detected.Motivation and Context
Currently, there's no enforcement that built JS files are included when the source changes (or if build tooling alters the build). This seeks to provide some protection that build files are kept up to date with those changes.
How Has This Been Tested?
See the commit history & screen shots below.
Screenshots (if appropriate):
Types of changes
Dev Tooling