-
Notifications
You must be signed in to change notification settings - Fork 54
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 linter CI #1301
Add linter CI #1301
Conversation
8abd49a
to
5c2f358
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1301 +/- ##
=======================================
Coverage 56.32% 56.32%
=======================================
Files 327 327
Lines 11944 11944
Branches 2741 2741
=======================================
Hits 6728 6728
Misses 5171 5171
Partials 45 45
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Hi @mengweieric, thanks for putting this together. I just left some comments
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.
LGTM!
working-directory: OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }} | ||
run: | | ||
git fetch origin main | ||
CHANGED_FILES=$(git diff --name-only --diff-filter=ACMRTUXB origin/main | grep -E "\.(js|ts|tsx)$") |
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.
why --diff-filter
? seems unnecessary
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 is for not including files that are deleted, and also with this it explicitly shows what are included and what are not. I actually tried before to run CI without these flags but it was getting errors, and it runs successfully after adding it back. without flags: https://github.com/mengweieric/dashboards-observability/actions/runs/7254224280/job/19762669894. Do you have any other suggestions that it can be written differently without those flags but do the work?
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.
if you just want to exclude deleted, it should be --diff-filter=d
?
also you are right it'll cause issues with deleted 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.
The --diff-filter we specify here are actually specifying what we select from git diff if I'm not mistaken: https://git-scm.com/docs/git-diff#Documentation/git-diff.txt---diff-filterACDMRTUXB82308203, therefore here without 'D' we skipped deleted files but select all others.
7e6a421
to
84f4cc1
Compare
3ebf9e4
to
280a8d7
Compare
Signed-off-by: Eric <menwe@amazon.com>
* linter CI (#1301) Signed-off-by: Eric <menwe@amazon.com> * change version to 2.x Signed-off-by: Eric <menwe@amazon.com> * add missing scripts Signed-off-by: Eric <menwe@amazon.com> --------- Signed-off-by: Eric <menwe@amazon.com>
Signed-off-by: Eric <menwe@amazon.com>
Signed-off-by: Ryan Liang <jiallian@amazon.com>
* linter CI (opensearch-project#1301) Signed-off-by: Eric <menwe@amazon.com> * change version to 2.x Signed-off-by: Eric <menwe@amazon.com> * add missing scripts Signed-off-by: Eric <menwe@amazon.com> --------- Signed-off-by: Eric <menwe@amazon.com> (cherry picked from commit 138bd21)
Description
Add linter CI to lint only changed files including
The linting will be ran only on changes matching above patterns in that PR.
Sample linting run result
https://github.com/mengweieric/dashboards-observability/actions/runs/7215956806/job/19661217812#step:8:1
Sample annotation when linting errors found
mengweieric@3052873#diff-2790e5916efcabbaa1ad67efe53928cc72a31ecdf3094cfd0c9582471f91530eL85
Issues Resolved
[List any issues this PR will resolve]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.