-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add CSV coverage PR commenter #6028
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 CSV coverage PR commenter #6028
Conversation
e162b91
to
5a8aa33
Compare
5a8aa33
to
aa0fa87
Compare
3f13ff7
to
3685194
Compare
922e953
to
a789d5a
Compare
a789d5a
to
7bb499c
Compare
javaGenerated files are not matching for java
|
Minor changes suggested below (add highlighting, and change wording from "mismatch" to "changes" to avoid giving the impression that there is something wrong). javaGenerated file changes for java
- `Spring <https://spring.io/>`_,``org.springframework.*``,30,,14,,,,,14,,
+ `Spring <https://spring.io/>`_,``org.springframework.*``,29,,14,,,,,14,,
- Totals,,85,1233,121,13,6,6,,33,1,2
+ Totals,,84,1233,121,13,6,6,,33,1,2
- org.springframework.security.web.savedrequest,,7,,,,,,,,,,,,,7,,
+ org.springframework.security.web.savedrequest,,6,,,,,,,,,,,,,6,, |
ab6518b
to
d65e260
Compare
b0496a0
to
0170004
Compare
javaGenerated file changes for java
- `Spring <https://spring.io/>`_,``org.springframework.*``,30,,14,,,,,14,,
+ `Spring <https://spring.io/>`_,``org.springframework.*``,29,,14,,,,,14,,
- Totals,,85,1233,121,13,6,6,,33,1,2
+ Totals,,84,1233,121,13,6,6,,33,1,2
- org.springframework.security.web.savedrequest,,7,,,,,,,,,,,,,7,,
+ org.springframework.security.web.savedrequest,,6,,,,,,,,,,,,,6,, |
f420a41
to
aaa07d7
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.
Nice! Some suggestions on the details of the Actions workflows, but the general setup looks good.
aaa07d7
to
4d9003b
Compare
Thanks @adityasharad for the code review. Here's a PR comment generated after the fixes. Also, here's the latest timeseries run. |
# "' match the ones in the codebase.") | ||
|
||
# if not all_ok: | ||
# sys.exit(1) |
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.
There is lots of commented-out code above this line. Should that be removed?
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.
Yes, I'll remove these, and some of it will come back as part of the followup work on the automatic PR creation with the changes in main
.
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.
Actions code looks good, only minor suggestions remaining.
Co-authored-by: Aditya Sharad <6874315+adityasharad@users.noreply.github.com>
5bab725
to
07b83d5
Compare
This PR adds a github action which runs on PRs. It compares the merge commit of the PR and the head of the base branch. On both commits the CSV reports are generated, and the differences are added as a comment on the PR. The workflow is split into two parts:
on: pull_request
to generate the reports andon: workflow_run
to compare them and add a PR comment.The PR is based on #6008, so only the last commit(s) should be reviewed.
I've tested the commenting functionality in https://github.com/dsp-testing/codeql-csv-coverage-pr-commenter/pull/1.