Skip to content
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

🐛 Fix for repos which do not squash PR commits #1637

Merged
merged 1 commit into from
Feb 14, 2022
Merged

Conversation

azeemshaikh38
Copy link
Contributor

  • Please check if the PR fulfills these requirements
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Part of Commit based scorecard #575. Repos which do not squash their PR commits end up with multiple commits associated with a single PR. This PR fixes that.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    No.

Copy link
Member

@naveensrinivasan naveensrinivasan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, we would test for this.

@azeemshaikh38 azeemshaikh38 temporarily deployed to integration-test February 14, 2022 21:55 Inactive
@github-actions
Copy link

@naveensrinivasan
Copy link
Member

Disabled Auto merge.

@azeemshaikh38 Can we add tests for this PR?

@evverx
Copy link
Contributor

evverx commented Feb 14, 2022

Thanks! Looks like the issue is gone. I think it would be great if scorecard could show reviewed commits with --verbosity Debug as well because as far as I can remember I saw projects where PRs weren't reviewed even though scorecard thought they were. It was before the commit-based analysis was introduced though.

@evverx
Copy link
Contributor

evverx commented Feb 14, 2022

Just to clarify, more generally, what I'm trying to say is that it would be great if it was possible to kind of keep track of what scorecard does with --verbosity Debug (or something like that).

@azeemshaikh38 azeemshaikh38 temporarily deployed to integration-test February 14, 2022 22:56 Inactive
@azeemshaikh38 azeemshaikh38 temporarily deployed to integration-test February 14, 2022 22:58 Inactive
@azeemshaikh38
Copy link
Contributor Author

Disabled Auto merge.

@azeemshaikh38 Can we add tests for this PR?

Added some basic unit tests.

@github-actions
Copy link

@github-actions
Copy link

@azeemshaikh38 azeemshaikh38 temporarily deployed to integration-test February 14, 2022 23:05 Inactive
@azeemshaikh38 azeemshaikh38 temporarily deployed to integration-test February 14, 2022 23:06 Inactive
@github-actions
Copy link

@github-actions
Copy link

@azeemshaikh38
Copy link
Contributor Author

Thanks! Looks like the issue is gone. I think it would be great if scorecard could show reviewed commits with --verbosity Debug as well because as far as I can remember I saw projects where PRs weren't reviewed even though scorecard thought they were. It was before the commit-based analysis was introduced though.

I think this exists already and this PR improves the log messaging a bit more.

Just to clarify, more generally, what I'm trying to say is that it would be great if it was possible to kind of keep track of what scorecard does with --verbosity Debug (or something like that).

This is hard since the code is going through refactoring and sometimes we lose some debug messages. Although if you let us know what specific messages help we can look into adding those messages. Also, as always, PRs are welcome :)

@azeemshaikh38 azeemshaikh38 enabled auto-merge (squash) February 14, 2022 23:10
@azeemshaikh38 azeemshaikh38 temporarily deployed to integration-test February 14, 2022 23:12 Inactive
@github-actions
Copy link

@azeemshaikh38 azeemshaikh38 merged commit 1e488a8 into main Feb 14, 2022
@azeemshaikh38 azeemshaikh38 deleted the azeems/systemd branch February 14, 2022 23:33
@evverx
Copy link
Contributor

evverx commented Feb 15, 2022

I think this exists already and this PR improves the log messaging a bit more.

It appears it does. But I ran it with --verbosity Debug (which was supported and is still documented at https://github.com/ossf/scorecard/blob/main/checks/write.md#how-to-write-a-check) but it seems when scorecard switched to another logger it broke Debug:

./scorecard  --repo https://github.com/systemd/systemd --verbosity Debug --show-details --checks Code-Review --format json |& jq
{
  "date": "2022-02-14",
  "repo": {
    "name": "github.com/systemd/systemd",
    "commit": "b6fc52408afa91f2fb7650e6a7d42c65396e7815"
  },
  "scorecard": {
    "version": "4.0.1-88-g1e488a8",
    "commit": "1e488a804fa102dffca444e84f7c8ffbedbb3bce"
  },
  "score": 10.0,
  "checks": [
    {
      "details": null,
      "score": 10,
      "reason": "all last 30 commits are reviewed through GitHub",
      "name": "Code-Review",
      "documentation": {
        "url": "https://github.com/ossf/scorecard/blob/1e488a804fa102dffca444e84f7c8ffbedbb3bce/docs/checks.md#code-review",
        "short": "Determines if the project requires code review before pull requests (aka merge requests) are merged."
      }
    }
  ],
  "metadata": null
}

With debug instead of Debug it works:

{
  "date": "2022-02-14",
  "repo": {
    "name": "github.com/systemd/systemd",
    "commit": "b6fc52408afa91f2fb7650e6a7d42c65396e7815"
  },
  "scorecard": {
    "version": "4.0.1-88-g1e488a8",
    "commit": "1e488a804fa102dffca444e84f7c8ffbedbb3bce"
  },
  "score": 10.0,
  "checks": [
    {
      "details": [
        "Debug: commit b6fc52408afa91f2fb7650e6a7d42c65396e7815 was reviewed through GitHub #22510 approved merge request",
        "Debug: commit f3376ee8fa28aab3f7edfad1ddfbcceca5bc841c was reviewed through GitHub #22487 approved merge request",
        "Debug: commit 88586e5d3217b681af52abb7a8f0ffe52a20a048 was reviewed through GitHub #22508 approved merge request",
        "Debug: commit d1e7fa02ca974c1f10930854fae66c90e16de189 was reviewed through GitHub #22507 approved merge request",
        "Debug: commit 1d7150ec7fd819805b2a19b1dd485be7de54a6a8 was reviewed through GitHub #22509 approved merge request",
        "Debug: commit d6b218e7426ef78e1682959c37b015e6eb692dc5 was reviewed through GitHub #22508 approved merge request",
        "Debug: commit c20c77eff8ef1a76ef5aea02df7121507bf9be69 was reviewed through GitHub #22508 approved merge request",
        "Debug: commit 675e7fc22c7ab45bf9f4a4414bf7c87734a00d15 was reviewed through GitHub #22508 approved merge request",
        "Debug: commit a9dac7a6dd31225dbe9633061dcade12c0c90a32 was reviewed through GitHub #22508 approved merge request",
        "Debug: commit 38db6211b5aa6e2fb65883ae3353914e82f5bef1 was reviewed through GitHub #22508 approved merge request",
        "Debug: commit d74da762a3b1edbb7935c3b3a229db601f734125 was reviewed through GitHub #22506 approved merge request",
        "Debug: commit bfba9946a1cd9f9219de9656e47c3ceb534a7bb0 was reviewed through GitHub #22445 approved merge request",
        "Debug: commit d5ac1d4e10e6bec3ab63cd95fb3b729e3e5d1d96 was reviewed through GitHub #22487 approved merge request",
        "Debug: commit f63b5ad93592ff64b5e8a83b63f2a3daade28114 was reviewed through GitHub #22487 approved merge request",
        "Debug: commit 56350400918c6979c60d46b7825e9671ee31f09c was reviewed through GitHub #22487 approved merge request",
        "Debug: commit bb6820576870d0b38dbf9f6e489b126558fc87d9 was reviewed through GitHub #22487 approved merge request",
        "Debug: commit a78e472dfd7832a13e3d52797e672d4e77fc2a49 was reviewed through GitHub #22487 approved merge request",
        "Debug: commit d403d8f0d65bdcecf7555a0cf040aa6de3de666e was reviewed through GitHub #22487 approved merge request",
        "Debug: commit fdc5c0429982a0b072736f0031bd61caf48a4193 was reviewed through GitHub #22487 approved merge request",
        "Debug: commit 4cddc18d0ac8127b8cad47d250563dc791c41c10 was reviewed through GitHub #22487 approved merge request",
        "Debug: commit 736783d42035e0e654508cf3ea0ae95e76dbc1f5 was reviewed through GitHub #22487 approved merge request",
        "Debug: commit 9951736b7fe532f266ad8457a889047e1396fb76 was reviewed through GitHub #22487 approved merge request",
        "Debug: commit 93e0d3204c76618c21d7eac1a4952af0a96c3cb9 was reviewed through GitHub #22507 approved merge request",
        "Debug: commit fe9bd5ad3670f6a34f9ea9b4e2c16bec6000ce11 was reviewed through GitHub #22507 approved merge request",
        "Debug: commit e46433bb92e0db1c87591c6dc5e280ceec2842c6 was reviewed through GitHub #22507 approved merge request",
        "Debug: commit 75a505c6006f9ddf536b426941ace1f87b263399 was reviewed through GitHub #22507 approved merge request",
        "Debug: commit b74163607b4e513288d38b7f16e230f5a61400c0 was reviewed through GitHub #22507 approved merge request",
        "Debug: commit fdf9de694f4c55b8cebd9d749f9c17c4bfb32650 was reviewed through GitHub #22505 approved merge request",
        "Debug: commit 42672c80dc9d4d3f918e2e6e49f064155c90d8e4 was reviewed through GitHub #22501 approved merge request",
        "Debug: commit d0ebe2a83579f1f6a300dc50882e99c6018dda01 was reviewed through GitHub #22496 approved merge request"
      ],
      "score": 10,
      "reason": "all last 30 commits are reviewed through GitHub",
      "name": "Code-Review",
      "documentation": {
        "url": "https://github.com/ossf/scorecard/blob/1e488a804fa102dffca444e84f7c8ffbedbb3bce/docs/checks.md#code-review",
        "short": "Determines if the project requires code review before pull requests (aka merge requests) are merged."
      }
    }
  ],
  "metadata": null
}

@evverx
Copy link
Contributor

evverx commented Feb 15, 2022

I don't think I have a lot of bash scripts I have to fix but it would be great if it was possible to keep the command line interface backward compatible going forward (or at least it should fail hard if a flag it can't handle is passed to it)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants