🐛 Fix branch protection results

[laurentsimon]

BP results are wrong in certain cases.
See #1247 and #1246

I think this PR fixes it. There may be a cleaner way to re-arrange the code, but for now this should be enough for a quick fix.

During my tests, I found that a read-only non-admin token could retrieve all the results. I need someone else to confirm.

[laurentsimon]

@evverx Can you try this PR and see if it fixes your problem?

[evverx]

Thanks! Looks like #1246 is gone but #1247 isn't

{
  "date": "2021-11-11",
  "repo": {
    "name": "github.com/evverx/systemd",
    "commit": "e127b048e6a2e2b8ad195385b0620d42c9515cc5"
  },
  "scorecard": {
    "version": "3.1.1-53-g2cf6047",
    "commit": "2cf60476a04647160cb12f0f27fe9b6ddcba66f3"
  },
  "score": 4.0,
  "checks": [
    {
      "details": [
        "Warn: 'force pushes' enabled on branch 'main'",
        "Info: 'allow deletion' disabled on branch 'main'",
        "Warn: linear history disabled on branch 'main'",
        "Warn: 'administrator' PRs are exempt from reviews on branch 'main'",
        "Info: strict status check enabled on branch 'main'",
        "Warn: status checks for merging have no specific status to check on branch 'main'",
        "Info: number of required reviewers is 2 on branch 'main'",
        "Info: Stale review dismissal enabled on branch 'main'",
        "Warn: Owner review not required on branch 'main'"
      ],
      "score": 4,
      "reason": "branch protection is not maximal on development and all release branches",
      "name": "Branch-Protection",
      "documentation": {
        "url": "https://github.com/ossf/scorecard/blob/2cf60476a04647160cb12f0f27fe9b6ddcba66f3/docs/checks.md#branch-protection",
        "short": "Determines if the default and release branches are protected with GitHub's branch protection settings."
      }
    }
  ],
  "metadata": null
}

curl  -u "***" -H "Accept: application/vnd.github.v3+json"   https://api.github.com/repos/evverx/systemd/branches/main/protection/required_status_checks
{
  "message": "Required status checks not enabled",
  "documentation_url": "https://docs.github.com/rest/reference/repos#get-status-checks-protection"
}

[laurentsimon]

Please try the last commit I've pushed. It's fixed for me now. Tell me if it's fixed on your side.

[evverx]

Judging by

...
  "score": 4.0,
  "checks": [
    {
      "details": [
        "Warn: 'force pushes' enabled on branch 'main'",
        "Info: 'allow deletion' disabled on branch 'main'",
        "Warn: linear history disabled on branch 'main'",
        "Warn: 'administrator' PRs are exempt from reviews on branch 'main'",
        "Info: status check disabled on branch 'main'",
        "Info: number of required reviewers is 2 on branch 'main'",
        "Info: Stale review dismissal enabled on branch 'main'",
        "Warn: Owner review not required on branch 'main'"
      ],
      "score": 4,
      ...
}

the issues are gone but I think it should be "Warn: status check disabled on branch 'main'". My understanding is that the "Info" messages are somewhat positive and increase the score while in this case it shouldn't I think.

[laurentsimon]

I fixed the Info -> Warn. Let me know if it works on your end. You're correct that Info are positive and Warn are negative.
Once we have the warnings right, I'll work on score calculation.

Thank you so much for your patience and help!

[evverx]

Thanks! It seems to be working. I'll go ahead and close those issues

[laurentsimon]

Thanks! It seems to be working. I'll go ahead and close those issues

Great. I may ping you once I got the score fixed. In a few days :-)

[laurentsimon]

@azeemsgoogle if you have time over the weekend, feel free to take over this PR. What's left to do is make the unit tests pass. To be honest, I don't think if it's worth the effort because I'm going to update the score calculation next week (discussion we had).

[azeemshaikh38]

Overall LGTM. My only concern is removing RefUpdateRule.

[laurentsimon]

I would like #1274 to be resolved before merging this PR.

[laurentsimon]

@azeemsgoogle I've temporarily commented out the unit tests for this check because I'm going to change the score calculation in the next PR. Fixing them now and once again in next PR is unnecessary work I'd like to avoid.
if everything else is fine, please LGTM.

[laurentsimon]

friendly ping. I only commented out the unit tests, not the e2e ones.

[azeemshaikh38]

@azeemsgoogle I've temporarily commented out the unit tests for this check because I'm going to change the score calculation in the next PR. Fixing them now and once again in next PR is unnecessary work I'd like to avoid. if everything else is fine, please LGTM.

I have given LGTM. I'm hesitant about submitting this with all unit tests disabled though. Its not a practice we as a project should be encouraging. Please see if you can re-enable some of the basic test which check for things like nil-ptr dereference. If not, would be good if we have the follow up PR soon.

[laurentsimon]

@azeemsgoogle I've temporarily commented out the unit tests for this check because I'm going to change the score calculation in the next PR. Fixing them now and once again in next PR is unnecessary work I'd like to avoid. if everything else is fine, please LGTM.

I have given LGTM. I'm hesitant about submitting this with all unit tests disabled though. Its not a practice we as a project should be encouraging. Please see if you can re-enable some of the basic test which check for things like nil-ptr dereference. If not, would be good if we have the follow up PR soon.

I intend to make it my next PR. Thanks!