-
Notifications
You must be signed in to change notification settings - Fork 37
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
Minder to avoid commenting each time there's a change on a PR #1862
Comments
Hi @rdimitrov. Currently, when Minder posts a review with vulnerabilities on GitHub (example: #1806 (review)), it adds one comment for each vulnerability and posts all of them alongside the review POST. However, the single-review approach will require comments to be deleted, edited, or added with each new PR (assuming vulnerabilities change). The issue is that the GitHub API doesn't support batch deleting or adding comments to an existing PR review, requiring one request per comment (DELETE/POST/PUT) running the risk of triggering rate limiting. Assuming suggestion comments are not entirely necessary, I think a some good alternative approaches could be:
However, if rate limiting is not going to be an issue then I'm happy to go with the 1 request per comment change approach. Else perhaps just deleting the previous review and posting a new one is the best approach. |
Given that finding CVEs in dependencies in a relatively rare thing I wouldn't stress too much about depleting the API tokens honestly. As long as the happy path (checking if dependencies have CVEs) don't trigger that many API calls, I would be OK to make the experience for the rarer use-case nicer. The alternative of deleting a previous review sounds honestly also good to me if API rate limiting is of concern. My 2c. |
Alright will keep going with this approach then.
Only consideration here is it runs the risk of being a bit spammy if one has email notifications enabled -- since each new review will send another notification. Not a huge deal though but could maybe get a bit annoying having 2 new emails/notifications for every new update. |
The motivation behind the issue is mostly to not clutter a PR that is in active development but also to avoid spamming all subscribed people since each review translates to another email. I think a nice balance can be to fix the summary so it is a single comment(instead of multiple new ones) which sits on top (assuming Minder commented first when the PR was created) and just edit it each time there's a change. This will help reduce the amount of clutter and spam.
@jhrozek @gregfurman - what do you think? |
Makes sense. So to clarify, the comments I am referring to are those that reference where in the diff a vulnerable dependency is imported. You are suggesting such comments be removed in favour of a summary table that will reside in the review body? So instead of a review with attached comments like in #1806 (review), there will be a table that includes this information within the review body without any review comments? |
If I understand @rdimitrov correctly, he's referring to the happy path where no CVEs are found in a patch that brings new dependencies. I still think if inline comments are nice. |
Apologies for the confusion. Indeed, I was referring to the happy path of no issues. Otherwise we should still do a review with inline comments. I'm thinking of the following use cases (do chime in if something sounds odd): 1. Minder found issues
2. Minder found no issues (so called happy path)
In both cases there should be only one comment from Minder in the PR. When evaluating for the 1st time we'll create the comment, but on next passes we'll just edit it to reflect the desired state (if needed) and either perform/discard a review. |
Alright so if I am understanding this correctly, you are suggesting:
So one should expect (in the case of a vulnerabilities being found) a single comment at the top of the PR to be edited and a separate review to be posted with inline comments. In the happy path, just expect the comment on top of the PR to be edited, Think I am getting a bit confused about the difference between a comment and a review in this context. Github unfortunately does not allow for new inline comments to be added to a review that has been posted -- one can only modify the review body (which is also technically a comment) and the text of the comments within the review. |
*and mark as resolved all the diff inline suggestions. The reason is Minder should not falsely block the PR in case the setting where a PR cannot be merged with unresolved discussions is enabled for a given repository.
That's correct! 👍
No, you're right, it's a bit awkward because indeed each review is essentially another comment 😃 By the way, feel free to ping me on Discord, I'll be happy to have a chat with you about this 👍 🙏 |
What are the thoughts on the below formatting for the pinned comment that gets edited? These were generated from the unit tests I've created and run locally. There is a dummy review referred to here but that should link to the actual minder review posted. Have also added some metadata to the body (in comment form) so that one can get the previous results of a scan and calculate how results differ from one commit to another (have not added this functionality in yet but the metadata is there). Vulnerabilities found example comment:Minder Vulnerability Report
|
Package | Version | #Vulnerabilities | #Fixes | Patch Exists |
---|---|---|---|---|
mongodb | 0.5.0 | 1 | 1 | ✅ |
No vulnerabilties found example comment:
Minder Vulnerability Report ✅
Minder analyzed this PR and found no vulnerable dependencies.
Vulnerability scan of
27d6810b
:
- 🐞 vulnerabilities:
0
- 🛠 fixes:
0
@gregfurman - Oh, this looks nice! 🚀 These will be a summary of all found packages, right?
Also, I wonder if we should also add a get in touch part like in the screenshot for Codecov? For example: Have feedback on the report? Share it here. |
Yeah, this summary just shows the total count of vulnerable dependencies and total count of suggested/available remediations. The table will go into more detail about how many vulnerabilities were found in each dependency. Happy to add more info here if it's unclear -- was trying to keep things simple and understandable 😄
That is a great idea. Perhaps it would be useful to provide another template to provide feedback? Labelling the issue as
|
I think the mail should be fine for now since we already refer to it from other things like the security advisories we alert through 👍 We can easily revisit if we want to converge on something else 👍 |
Please describe the enhancement
Each time there's a push to a PR triggers a new evaluation which results in another comment being added on the PR from minder. That can easily spam the PR and annoy people, i.e. #1806.
Solution Proposal
I'll give codecov as an example, but there are other tools that use this approach too.
The idea is to have only one comment that is just being edited each time there's a change.
Here's an example of this in a PR:
Describe alternatives you've considered
No response
Additional context
No response
Acceptance Criteria
No response
The text was updated successfully, but these errors were encountered: