BUG: RepoClient data format

[azeemshaikh38]

Currently, the RepoClient data format uses primitive Golang types. This means that when certain values are not returned, the result struct will have default values set. This can lead to incorrect results.

We should update the RepoClient data to use *bool, *string etc. And the convention will be that if the value is non-nil, only then analyze the resut.

[evverx]

Could it be that this causes #1247?

[azeemshaikh38]

Most likely yes. I have a PR out to fix this for Branch-Protection: #1243. With this PR, this is the result I get:

scorecard  --format json --show-details --verbosity Debug --checks Branch-Protection  --repo=https://github.com/evverx/systemd | jq
{
  "date": "2021-11-11",
  "repo": {
    "name": "github.com/evverx/systemd",
    "commit": "1b1e6249a0987820cde646a11c1b559dcb02d62f"
  },
  "scorecard": {
    "version": "3.0.1",
    "commit": "6c1c789dc5b05cde492334f57b53807c786b038a"
  },
  "score": 2,
  "checks": [
    {
      "details": [
        "Info: 'force pushes' disabled on branch 'main'",
        "Info: 'allow deletion' disabled on branch 'main'",
        "Warn: linear history disabled on branch 'main'",
        "Warn: status checks for merging disabled on branch 'main'",
        "Info: number of required reviewers is 2 on branch 'main'",
        "Warn: Stale review dismissal disabled on branch 'main'",
        "Warn: Owner review not required on branch 'main'",
        "Warn: 'admininistrator' PRs are exempt from reviews on branch 'main'"
      ],
      "score": 2,
      "reason": "branch protection is not maximal on development and all release branches",
      "name": "Branch-Protection",
      "documentation": {
        "url": "https://github.com/ossf/scorecard/blob/6c1c789dc5b05cde492334f57b53807c786b038a/docs/checks.md#branch-protection",
        "short": "Determines if the default and release branches are protected with GitHub's branch protection settings."
      }
    }
  ],
  "metadata": null
}

Does that align with your expected Scorecard output?

[evverx]

It does but with #1243 applied I got

{
  "date": "2021-11-11",
  "repo": {
    "name": "github.com/evverx/systemd",
    "commit": "1b1e6249a0987820cde646a11c1b559dcb02d62f"
  },
  "scorecard": {
    "version": "3.1.1-46-ga4afb71",
    "commit": "a4afb711e30eb368d3ecfa8b38d4c4efb62ca076"
  },
  "score": 4.0,
  "checks": [
    {
      "details": [
        "Info: 'force pushes' disabled 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'",
        "Warn: number of required reviewers is only 824638264608 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/a4afb711e30eb368d3ecfa8b38d4c4efb62ca076/docs/checks.md#branch-protection",
        "short": "Determines if the default and release branches are protected with GitHub's branch protection settings."
      }
    }
  ],
  "metadata": null
}

Warn: number of required reviewers is only 824638264608 on branch 'main' and Info: strict status check enabled on branch 'main' look weird I'd say :-) Let me try to rebuild scorecard anew with that PR

[evverx]

Looks like in your version of scorecard both #1247 and #1246 are fixed (but it somehow lost the "Stale review dismissal" subcheck). In my version of scorecard the two issues are somehow still present but manifest themselves a bit differently.

[evverx]

I'm not sure if that helps but just in case

$ go version
go version go1.17.3 linux/amd64

$ cat /proc/version
Linux version 5.14.16-101.fc33.x86_64 ([email protected]) (gcc (GCC) 10.3.1 20210422 (Red Hat 10.3.1-1), GNU ld version 2.35-18.fc33) #1 SMP Thu Nov 4 01:35:22 UTC 2021

[azeemshaikh38]

Ok, another great catch @evverx!

• number of required reviewers was a logging bug, I was logging the pointer value instead of the data.
• strict status check enabled was mostly due to the fact that I was copying over pointer and not data. Fixed that, let me know if that works now.
• stale review dismissal not showing up is likely expected. You need an admin token for the repo to get the value about stale review dismissal. If a non-admin runs the check, no value is returned about whether stale review dismissals are enabled or not. So we simply skip it in 🐛 Ignore nil values in Branch-Protection check #1243.

Hope that helps.

[azeemshaikh38]

Closing this.

[cpanato]

Is this still relevant?