-
Notifications
You must be signed in to change notification settings - Fork 510
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 probes for Branch Protection #3691
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3691 +/- ##
==========================================
+ Coverage 66.68% 68.91% +2.23%
==========================================
Files 217 227 +10
Lines 15061 15347 +286
==========================================
+ Hits 10043 10577 +534
+ Misses 4434 4124 -310
- Partials 584 646 +62 |
This pull request is stale because it has been open for 10 days with no activity |
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.
Most of the reviewing time was spent reading the yaml definitions. I think some of these branch protection settings are easier to do than others, so I tried to leave comments on the ones I think are low/medium effort.
There were comments that applied to most of these probes, so I got tired of leaving them. They generally were:
- mentioning the branch protection rules in general are only for default/release branches.
- the need to mention outcome not available if relevant
- some of the remediation is specific, some of it isn't. Need to figure out what @laurentsimon wants in terms of level of detail.
In terms of the implementations, there's a huge amount of code duplication problem. This is mainly targeted at the ones whose existence is just a bool pointer. They can be extracted to some helper to cut down on duplication. Some of these checks have more complicated scenarios, so I realize they won't use the helper.
In terms of testing, I prefer the names reflect the behavior being tested, not describing the test. In general, this had to do with the OutcomeNotAvailable not being reflected in the testname, and the test was doing more than one thing.
7270239
to
dd371e3
Compare
I added this helper here: dd371e3. WDYT? |
f2713cf
to
484862f
Compare
cf0feeb
to
2854e17
Compare
2854e17
to
4071df4
Compare
Signed-off-by: Adam Korczynski <[email protected]>
Signed-off-by: Adam Korczynski <[email protected]>
Signed-off-by: Adam Korczynski <[email protected]>
Signed-off-by: Adam Korczynski <[email protected]>
cc52c19
to
4bd1ec4
Compare
Signed-off-by: Adam Korczynski <[email protected]>
Signed-off-by: Adam Korczynski <[email protected]>
Signed-off-by: Adam Korczynski <[email protected]>
…eference Signed-off-by: Adam Korczynski <[email protected]>
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 may be some polish needed, but since it's not hooked up at this point, we'll have to wait until merge and next PR.
Just to be explicit, do you plan on doing the PR required probe in this one? a separate PR? or in the connecting PR?
In a separate PR. |
Signed-off-by: Adam Korczynski <[email protected]>
What kind of change does this PR introduce?
Feature
What is the new behavior (if this is a feature change)?**
This adds 9 probes for the Branch Protection check.
Which issue(s) this PR fixes
None
Special notes for your reviewer
The probes are not invoked anywhere by Scorecard; the PR does not change the evaluation. I will do that in a follow-up PR once this has landed.
Does this PR introduce a user-facing change?
NONE