-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
ci: add eslint check to only check changed files and fail when warnings are thrown #5357
Conversation
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
…yChangedFileLinting
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
@mikemurray could you please see that this works as it should, since you know what it's supposed to do? @aldeed / @focusaurus could you please check the bash script / config file code? |
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 pushed a commit to refactor the bash script a bit. You'll need to re-test nothing broke but I'm fairly confident it will still work properly. Otherwise looks good.
- format with shfmt - safer curl args - Use `jq -r` to remove quotes - consistent use of curlies around variable names Signed-off-by: Peter Lyons <[email protected]>
0eb643c
to
5483142
Compare
Signed-off-by: Erik Kieckhafer <[email protected]>
@mikemurray confirmed @focusaurus's changes still work. Here is the latest CI run that is failing on purpose, should be good enough for testing: https://circleci.com/gh/reactioncommerce/reaction/49757?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-checks-link |
Signed-off-by: Erik Kieckhafer <[email protected]>
Impact: minor
Type: ci|performance
Issue
We currently have ~300
eslint
warnings related tojsdoc
.Solution
To incrementally fix all these issues. we will add a check to the CI process to throw an error if there are any
eslint
errors
ORwarnings
, only files you changed in your PR. In addition to only checking changed files, this is different than our traditionaleslint
CI check in that it will not pass even if there arewarnings
, where our traditional check only fails againsterrors
.Breaking changes
None
Testing
eslint
warning, and make a PR with said change. Two files witheslint
warnings:imports/plugins/included/product-variant/client/templates/products/productSettings/productSettings.js
client/modules/core/main.js
I made a few test commits with purposefully failing files, you can see the CI check running and failing when there are eslint
warnings
(not errors): https://circleci.com/gh/reactioncommerce/reaction/49620?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-checks-link