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

check if the PR is a fork PR #328

Merged
merged 4 commits into from
Nov 10, 2023
Merged

check if the PR is a fork PR #328

merged 4 commits into from
Nov 10, 2023

Conversation

dana-yaish
Copy link
Contributor

@dana-yaish dana-yaish commented Nov 7, 2023

This is the first PR for tokenless upload and branch conflation bug fix where we check if the PR is a fork PR.
I'm not using any auth tokens as forked PRs are public for open source repos. And a forked repo made from a public upstream cannot change its visibility to private.
This PR implements getting PR info from github only, other providers are needed still, but pushing my code to get initial review
We'll need to add the fork PR check in each command we want to support tokenless for. For uploads we'll need to add it to create_commit, create_report and do_upload commands logic. But, create_report does not have reference to the PR number, which means we'll need to add it there.

codecov/engineering-team#733

Copy link

codecov bot commented Nov 7, 2023

Codecov Report

Merging #328 (5ac1758) into main (5ed8261) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #328      +/-   ##
==========================================
+ Coverage   95.39%   95.46%   +0.06%     
==========================================
  Files          78       80       +2     
  Lines        2673     2713      +40     
==========================================
+ Hits         2550     2590      +40     
  Misses        123      123              
Flag Coverage Δ
python3.10 95.71% <100.00%> (+0.06%) ⬆️
python3.11 95.70% <100.00%> (+0.06%) ⬆️
python3.8 95.71% <100.00%> (+0.06%) ⬆️
python3.9 95.71% <100.00%> (+0.06%) ⬆️
smart-labels 95.45% <100.00%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
codecov_cli/helpers/encoder.py 100.00% <100.00%> (ø)
codecov_cli/helpers/git.py 100.00% <100.00%> (ø)
codecov_cli/helpers/git_services/__init__.py 100.00% <ø> (ø)
codecov_cli/helpers/git_services/github.py 100.00% <100.00%> (ø)
codecov_cli/services/commit/__init__.py 100.00% <100.00%> (ø)
codecov_cli/services/upload/upload_sender.py 100.00% <100.00%> (ø)

@dana-yaish dana-yaish force-pushed the dana/identify-fork-prs branch 2 times, most recently from 9ddc2dd to 6802c70 Compare November 7, 2023 11:24
matt-codecov
matt-codecov previously approved these changes Nov 8, 2023
Copy link
Contributor

@matt-codecov matt-codecov left a comment

Choose a reason for hiding this comment

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

lgtm, approving to spare you timezone hell but pls read the comments and decide how to address them

git_service = get_git_service(service)
if git_service:
pull_dict = git_service.get_pull_request(decoded_slug, pr_num)
if pull_dict and pull_dict["head"]["slug"] != decoded_slug:
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to decode the slug? if the PR API request gives us both the head and the base, we can just see if they're different?

if pull_dict and pull_dict["head"]["slug"] != pull_dict["base"]["slug"]:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right we ca do it both ways, but we need to decode it since we need it in the API request to get the pull request

codecov_cli/helpers/git_services/github.py Show resolved Hide resolved
@dana-yaish dana-yaish force-pushed the dana/identify-fork-prs branch from b5d0e5d to 0dfd717 Compare November 8, 2023 12:05
Copy link
Contributor

@matt-codecov matt-codecov left a comment

Choose a reason for hiding this comment

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

thanks for adding the test! other changes also look good

@dana-yaish dana-yaish merged commit 67efc57 into main Nov 10, 2023
14 checks passed
@dana-yaish dana-yaish deleted the dana/identify-fork-prs branch November 10, 2023 11:13
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.

make CLI aware if the PR is made from a fork repo
2 participants