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

🌱 Only return PRs associated with recent commits #1562

Merged
merged 1 commit into from
Jan 30, 2022

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, ...)
    Only returns PRs used to merge recent commits if any. None otherwise.

Fixes #1524. Part of #575.

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

@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2022

Codecov Report

Merging #1562 (f063f72) into main (53f21cb) will decrease coverage by 0.08%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1562      +/-   ##
==========================================
- Coverage   53.04%   52.96%   -0.09%     
==========================================
  Files          70       70              
  Lines        6155     6165      +10     
==========================================
  Hits         3265     3265              
- Misses       2683     2693      +10     
  Partials      207      207              

@azeemshaikh38
Copy link
Contributor Author

Ok I need some inputs here @laurentsimon @naveensrinivasan @justaugustus - if we get the PRs from commit.associatedPR instead of the repository, the behavior is different on some repos like forks. Basically, forks do not have any PRs, so GitHub APIs return no PRs on forks. But with commit.associatedPR, GitHub recognizes that the commit has a PR on the original repo so it returns that PR as part of commit.associatedPR. Now, the question is, is this a behavior we want?

I personally think this is the right behavior, since we want to analyze "commits" not PRs. Commits might come from Gerrit, Prow or a GitHub PR of a different repo and we should be able to recognize that. What do you folks think?

@azeemshaikh38 azeemshaikh38 temporarily deployed to integration-test January 30, 2022 02:33 Inactive
@github-actions
Copy link

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

@azeemshaikh38 azeemshaikh38 marked this pull request as ready for review January 30, 2022 02:53
@azeemshaikh38
Copy link
Contributor Author

Ok I need some inputs here @laurentsimon @naveensrinivasan @justaugustus - if we get the PRs from commit.associatedPR instead of the repository, the behavior is different on some repos like forks. Basically, forks do not have any PRs, so GitHub APIs return no PRs on forks. But with commit.associatedPR, GitHub recognizes that the commit has a PR on the original repo so it returns that PR as part of commit.associatedPR. Now, the question is, is this a behavior we want?

I personally think this is the right behavior, since we want to analyze "commits" not PRs. Commits might come from Gerrit, Prow or a GitHub PR of a different repo and we should be able to recognize that. What do you folks think?

Let's discuss further on #575. I was able to figure out how to keep the current behavior unchanged, but this is something we should consider.

@azeemshaikh38 azeemshaikh38 merged commit 58865e9 into main Jan 30, 2022
@azeemshaikh38 azeemshaikh38 deleted the azeems/commit branch January 30, 2022 02:55
@laurentsimon
Copy link
Contributor

Ok I need some inputs here @laurentsimon @naveensrinivasan @justaugustus - if we get the PRs from commit.associatedPR instead of the repository, the behavior is different on some repos like forks. Basically, forks do not have any PRs, so GitHub APIs return no PRs on forks. But with commit.associatedPR, GitHub recognizes that the commit has a PR on the original repo so it returns that PR as part of commit.associatedPR. Now, the question is, is this a behavior we want?

I personally think this is the right behavior, since we want to analyze "commits" not PRs. Commits might come from Gerrit, Prow or a GitHub PR of a different repo and we should be able to recognize that. What do you folks think?

LGTM at first sight.

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.

BUG: Checks using ListMergedPRs() need hardening
5 participants