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

✨ Raw data for code review check #1505

Merged
merged 24 commits into from
Feb 2, 2022

Conversation

laurentsimon
Copy link
Contributor

@laurentsimon laurentsimon commented Jan 21, 2022

Add support for raw results for Branch-Protection check.
No breaking changes.
The hope is that we will be able t re-use the "commit" structure for SAST.

Output:

"commits": [
      {
        "committer": {
          "login": "dependabot[bot]"
        },
        "review": {
          "platform": {
            "name": "GitHub"
          },
          "authors": [
            {
              "login": "azeemshaikh38"
            }
          ],
          "merge-request": {
            "number": 1459,
            "author": {
              "login": "dependabot"
            }
          }
        }
      },
      {
        "committer": {
          "login": "justaugustus"
        },
        "review": {
          "platform": {
            "name": "GitHub"
          },
          "authors": [
            {
              "login": "azeemshaikh38"
            }
          ],
          "merge-request": {
            "number": 1502,
            "author": {
              "login": "justaugustus"
            }
          }
        }
      }
...

cc @jeffmendoza @asraa to give you some idea of what the raw results look like at the moment.

checker/raw_result.go Outdated Show resolved Hide resolved
checker/raw_result.go Outdated Show resolved Hide resolved
checker/raw_result.go Outdated Show resolved Hide resolved
@laurentsimon laurentsimon temporarily deployed to integration-test January 21, 2022 18:32 Inactive
@laurentsimon laurentsimon temporarily deployed to integration-test January 21, 2022 18:33 Inactive
@github-actions
Copy link

Integration tests success for
[870905f]
(https://github.com/ossf/scorecard/actions/runs/1730196089)

@github-actions
Copy link

Integration tests success for
[284955b]
(https://github.com/ossf/scorecard/actions/runs/1730200303)

@laurentsimon laurentsimon temporarily deployed to integration-test January 21, 2022 19:32 Inactive
@github-actions
Copy link

Integration tests success for
[188f4e6]
(https://github.com/ossf/scorecard/actions/runs/1730433565)

@laurentsimon laurentsimon temporarily deployed to integration-test January 21, 2022 19:58 Inactive
@github-actions
Copy link

Integration tests success for
[e8069a3]
(https://github.com/ossf/scorecard/actions/runs/1730531963)

@azeemshaikh38
Copy link
Contributor

@laurentsimon will look at the PR over the weekend or maybe Monday. If this is blocking you in any way, let me know and I can prioritize this.

@laurentsimon
Copy link
Contributor Author

@laurentsimon will look at the PR over the weekend or maybe Monday. If this is blocking you in any way, let me know and I can prioritize this.

Monday's fine. I don't have work blocked on this PR, np.

checks/code_review.go Outdated Show resolved Hide resolved
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.

Tests are required for such a large feature change. We shouldn't merge up until that.

@laurentsimon
Copy link
Contributor Author

Tests are required for such a large feature change. We shouldn't merge up until that.

we won't. Btw, the existing unit tests still pass. This PR does not changing the logics, it just re-factors the code.

@github-actions
Copy link

Integration tests success for
[7c88215]
(https://github.com/ossf/scorecard/actions/runs/1757929451)

@laurentsimon laurentsimon temporarily deployed to integration-test February 2, 2022 04:01 Inactive
@laurentsimon laurentsimon temporarily deployed to integration-test February 2, 2022 17:42 Inactive
@github-actions
Copy link

github-actions bot commented Feb 2, 2022

Integration tests success for
[e156162]
(https://github.com/ossf/scorecard/actions/runs/1785344396)

@github-actions
Copy link

github-actions bot commented Feb 2, 2022

Integration tests success for
[4b78a9c]
(https://github.com/ossf/scorecard/actions/runs/1785346973)

@laurentsimon laurentsimon temporarily deployed to integration-test February 2, 2022 18:10 Inactive
@github-actions
Copy link

github-actions bot commented Feb 2, 2022

Integration tests success for
[ce8bf30]
(https://github.com/ossf/scorecard/actions/runs/1785467385)

@github-actions
Copy link

github-actions bot commented Feb 2, 2022

Integration tests success for
[00d1917]
(https://github.com/ossf/scorecard/actions/runs/1785468099)

@codecov-commenter
Copy link

codecov-commenter commented Feb 2, 2022

Codecov Report

Merging #1505 (2ad2c0e) into main (0670b8b) will decrease coverage by 2.59%.
The diff coverage is 1.83%.

@@            Coverage Diff             @@
##             main    #1505      +/-   ##
==========================================
- Coverage   54.42%   51.82%   -2.60%     
==========================================
  Files          70       72       +2     
  Lines        6175     6313     +138     
==========================================
- Hits         3361     3272      -89     
- Misses       2603     2836     +233     
+ Partials      211      205       -6     

@laurentsimon laurentsimon enabled auto-merge (squash) February 2, 2022 18:55
@laurentsimon laurentsimon temporarily deployed to integration-test February 2, 2022 18:55 Inactive
@naveensrinivasan
Copy link
Member

Codecov Report

Merging #1505 (00d1917) into main (009aa85) will decrease coverage by 2.59%.
The diff coverage is 1.83%.

@@            Coverage Diff             @@
##             main    #1505      +/-   ##
==========================================
- Coverage   54.42%   51.82%   -2.60%     
==========================================
  Files          70       72       +2     
  Lines        6175     6313     +138     
==========================================
- Hits         3361     3272      -89     
- Misses       2603     2836     +233     
+ Partials      211      205       -6     

@laurentsimon Disabled auto merge. How do we address this?

@github-actions
Copy link

github-actions bot commented Feb 2, 2022

Integration tests success for
[2ad2c0e]
(https://github.com/ossf/scorecard/actions/runs/1785668681)

@laurentsimon
Copy link
Contributor Author

laurentsimon commented Feb 2, 2022

The bulk of the coverage drops appears to be from file checks/raw/code_review.go which shows with no hits. I've re-factored the code but not changed the logic. Why does it show no coverage? It's literally telling us none of the lines were tested. Do you know what's going on?

@naveensrinivasan
Copy link
Member

The bulk of the coverage drops appears to be from file checks/raw/code_review.go which shows with no hits. I've re-factored the code but not changed the logic. Why does it show no coverage? It's literally telling us none of the lines were tested. Do you know what's going on?

TBH I don't know. If you think it is good, please merge it 👍

@laurentsimon
Copy link
Contributor Author

The bulk of the coverage drops appears to be from file checks/raw/code_review.go which shows with no hits. I've re-factored the code but not changed the logic. Why does it show no coverage? It's literally telling us none of the lines were tested. Do you know what's going on?

TBH I don't know. If you think it is good, please merge it 👍

something is off but I don't know why codecov does not see it :/ Let's merge and please remind me to see what the following PRs show about these lines.

@laurentsimon laurentsimon enabled auto-merge (squash) February 2, 2022 19:27
@laurentsimon laurentsimon temporarily deployed to integration-test February 2, 2022 19:28 Inactive
@github-actions
Copy link

github-actions bot commented Feb 2, 2022

Integration tests success for
[b9e798d]
(https://github.com/ossf/scorecard/actions/runs/1785788989)

@laurentsimon laurentsimon merged commit 9037444 into ossf:main Feb 2, 2022
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.

6 participants