-
Notifications
You must be signed in to change notification settings - Fork 482
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
✨ Automatic dependency update checks #322
Conversation
Integration tests cancelled for e5bc9e51134e8d0a4f5599645b4d1f9ddf6bdb68 |
Integration tests success for 13145736ba4fae9911e1d455739679ac6b6a70ec |
checks.md
Outdated
|
||
**Remediation steps**: | ||
- Enable dependabot for the project and have a dependabot.yml file in the repository. | ||
|
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 an amazing check, but there are two problems:
- Dependabot can be enabled on org level
- Dependabot can be enabled via UI
and this is substantial portion of things, and we would show incorrect result for these. maybe we should bring some of these problems with github api in next openssf sync, e.g. branch protection, this one with knowing security settings like dependabot, etc. Landing this as-is will be problematic since we cannot get right status in many cases and scorecard needs to be accurate. thoughts?
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 an amazing check, but there are two problems:
- Dependabot can be enabled on org level
- Dependabot can be enabled via UI
and this is substantial portion of things, and we would show incorrect result for these. maybe we should bring some of these problems with github api in next openssf sync, e.g. branch protection, this one with knowing security settings like dependabot, etc. Landing this as-is will be problematic since we cannot get right status in many cases and scorecard needs to be accurate. thoughts?
I agree this might be an issue. We can have in-depth discussion.
Also
-
But why not provide an results with lesser confidence, we can never be 100% in some of the cases.
-
Another way to solve this issues is to look for commits and check for
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
and check for signed commits
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 can be then
- look for that file .github/dependabot.yml, if exists, pass with high confidence.
- if failed, check for commits [we have to see how far we can go without burning quota, or instead see if we can get user dependabot events by api]. if that exists, pass with medium-high confidence, depending on last update?
- otherwise fail with low confidence (like 3?)
thoughts, can also brainstorm in next meeting with @azeemshaikh38 . but even this is a good start i think.
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.
Sounds good.
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.
Some projects may have dependabot for tooling (let's say a primary C++ project with no dependabot, but has python deps that utilize dependabot).
I wonder if we should lower confidence for projects with unsupported languages
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.
re: dependabot commits. If people have dependabot installed, we might still want to verify that they accept dependabot's PRs once in a while.
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 right, this might be a better or definitely needed part here, which is did any dependendabot cls land in a while.
Going to start working on this. |
re: @asraa I agree confidence should be lowered for other languages in the repo that are not defined in the dependabot config file. We could list files and their extension: if any of these files are not tracked by the dependabot config, they should reduce the score. It'd be great to weight the scoring based on how much code is tracked or not - maybe by looking at number of lines for each lanaguages. There's the language API that I initially thought could help, but it unfortunately only works at the repo level, ie we cannot specify folders of interest. |
@nathannaveen what's left for this PR to land? |
I think for the initial release I am not going to target that |
☝️ This will burn the API quota's . AFAIK there aren't dependabot events by API.
For now, we can stay with fail with low confidence. Thoughts? |
Ok this is fine, but please add renovatebot file check in this as well. renovatebot is extremely common, and either one of the two is fine. in documentation, we should say this check only looks if it is enabled and does not ensure that it is run and prs are merged (maybe tbd for future ?) ?? @azeemshaikh38 @laurentsimon thoughts? |
Please add a TODO in the code that we should also list commits even when we find the dependabot's config file (to ensure users actually update their dependencies). re: quota: An alternative approach could be to run the cron jobs with some checks turned off. Within a few days we could run different checks so that, every week, we have all the checks. Do we actually need the cron job to run every single day? (It's ideal, but is it really necessary?)
I suppose that's the best we can do now. Note that "fail with low confidence" achieves little from a user's point of view. If I use scorecard and I get this result, it's almost like a noop: I have no idea how to interpret it. @asraa who tests scorecard for envoy, so she may comment. |
☝️ This is would be a separate issue
|
e5bc9e5
to
f6f2601
Compare
Integration tests success for 63a9a8f116fc370f064f921a0f9e952d18527c08 |
Integration tests success for f6f2601fb228d22b8bb547751d26ac87b1f23555 |
da88985
to
ad070b6
Compare
Integration tests success for da88985558645d4952cbfdea518c1eadff91fc04 |
Integration tests success for ad070b619aed860c27bf9e3b7a6161fcc00a2758 |
Integration tests success for ea5cb0b8e49f299daf1f93cbfc8ab43ee1e39caf |
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.
Thanks a lot!
|
||
// AutomaticDependencyUpdate will check the repository if it contains Automatic dependency update. | ||
func AutomaticDependencyUpdate(c *checker.CheckRequest) checker.CheckResult { | ||
result := CheckIfFileExists(check, c, fileExists) |
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.
We need to later improve CheckIfFileExists so that it caches content and does not download again if it already exists. We have too many checks now that depend on files. @azeemshaikh38 fyi
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 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.
Commented on the issue. This is definitely an improvement we should work on, but a low priority IMO.
ea5cb0b
to
0eabcdc
Compare
Integration tests success for 0eabcdcbd113d18b41d75a94c289f639b3658eb2 |
Integration tests success for a3db8a4f80c8a76c02b8e89f2747f219bce21903 |
* Checks if the dependencies are automatically updated.
a3db8a4
to
71724ad
Compare
Integration tests success for 71724aded09154d1ed49849996519a62be12142b |
Integration tests success for da27f3b562eaa992e88a3283c4b0934e6eb1e3b5 |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature
What is the current behavior? (You can also link to an open issue here)
New check - Use of tools that update deps like dependabot #179
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
None
Other information:
None