codecov-api icon indicating copy to clipboard operation
codecov-api copied to clipboard

Tokenless V3

Open joseph-sentry opened this issue 9 months ago • 7 comments

Purpose/Motivation

We don't want to make GH API calls when doing tokenless anymore

Links to relevant tickets

https://github.com/codecov/engineering-team/issues/1574

Related: https://github.com/codecov/codecov-action/pull/1406

What does this PR do?

  • Remove GitHub API calls from TokenlessAuthentication

joseph-sentry avatar May 01 '24 15:05 joseph-sentry

Codecov Report

Attention: Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.45%. Comparing base (9b8d730) to head (eca8697).

:white_check_mark: All tests successful. No failed tests found.

@@            Coverage Diff             @@
##             main     #533      +/-   ##
==========================================
- Coverage   91.49%   91.45%   -0.04%     
==========================================
  Files         615      615              
  Lines       16370    16354      -16     
==========================================
- Hits        14977    14957      -20     
- Misses       1393     1397       +4     
Flag Coverage Δ
unit 91.45% <94.11%> (-0.04%) :arrow_down:
unit-latest-uploader 91.45% <94.11%> (-0.04%) :arrow_down:

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

Files Coverage Δ
upload/views/reports.py 100.00% <ø> (ø)
codecov_auth/authentication/repo_auth.py 96.87% <96.42%> (-1.63%) :arrow_down:
upload/views/commits.py 97.14% <83.33%> (-2.86%) :arrow_down:

:mega: Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

codecov-qa[bot] avatar May 01 '24 15:05 codecov-qa[bot]

Codecov Report

Attention: Patch coverage is 94.11765% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 91.45%. Comparing base (9b8d730) to head (eca8697).

:white_check_mark: All tests successful. No failed tests found :relaxed:

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #533      +/-   ##
==========================================
- Coverage   91.49%   91.45%   -0.04%     
==========================================
  Files         615      615              
  Lines       16370    16354      -16     
==========================================
- Hits        14977    14957      -20     
- Misses       1393     1397       +4     
Flag Coverage Δ
unit 91.45% <94.11%> (-0.04%) :arrow_down:
unit-latest-uploader 91.45% <94.11%> (-0.04%) :arrow_down:

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

Files Coverage Δ
upload/views/reports.py 100.00% <ø> (ø)
codecov_auth/authentication/repo_auth.py 96.87% <96.42%> (-1.63%) :arrow_down:
upload/views/commits.py 97.14% <83.33%> (-2.86%) :arrow_down:

Impacted file tree graph

codecov-public-qa[bot] avatar May 01 '24 15:05 codecov-public-qa[bot]

Codecov Report

Attention: Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.

Project coverage is 95.91%. Comparing base (9b8d730) to head (eca8697).

:white_check_mark: All tests successful. No failed tests found.

Files Patch % Lines
codecov_auth/authentication/repo_auth.py 96.42% 1 Missing :warning:
upload/views/commits.py 83.33% 1 Missing :warning:
Additional details and impacted files
@@               Coverage Diff                @@
##               main       #533        +/-   ##
================================================
- Coverage   95.92000   95.91000   -0.01000     
================================================
  Files           793        793                
  Lines         17688      17671        -17     
================================================
- Hits          16968      16949        -19     
- Misses          720        722         +2     
Flag Coverage Δ
unit 91.45% <94.11%> (-0.04%) :arrow_down:
unit-latest-uploader 91.45% <94.11%> (-0.04%) :arrow_down:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 01 '24 15:05 codecov[bot]

We need to be careful merging these changes to avoid re-introducing the bug of having an upload from a fork branch accidentally overwrite coverage in the upstream branch.

Considering the upload endpoints used by the CLI either we change the CLI to send the branch info every time (maybe through headers) OR we use the info we have to make the validation that the branch name is in the format fork:branch. If that is not the case, reject the upload.

Currently I believe it should be possible to make this check:

  • For the commit creation the branch should be part of the body
  • For the subsequent requests (create report and send the upload) we can pull the commit from the database and check its branch information.

Personally I feel that we should be able to accept / reject a request without having to look at the request body. So I'd opt to make the CLI send specific headers with the information we need. (Also by removing the current checks maybe X-Tokenless-PR header is useless?)

giovanni-guidini avatar May 02 '24 08:05 giovanni-guidini

So I'd opt to make the CLI send specific headers with the information we need. (Also by removing the current checks maybe X-Tokenless-PR header is useless?)

We could just validate that the x-tokenless-pr header is in the format we expect and has the correct repo name (it matches the one in the url)?

joseph-sentry avatar May 02 '24 13:05 joseph-sentry

X-Tokenless-PR is a number [1] that we used to get the correct PR from the provider. That does little for us considering the changes in this PR. If you just change the value the header name will be misleading.

X-Tokenless should not match the repo name in the URL if it comes from a fork [1], cause it's supposed to be the fork slug, while the slug in the URL should be the upstream's.

[1] https://github.com/codecov/codecov-cli/blob/7b028499a521f8df3cf3bb6b642246a19e07bee9/codecov_cli/services/report/init.py#L57-L58

giovanni-guidini avatar May 02 '24 14:05 giovanni-guidini

Codecov Report

Attention: Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.

:white_check_mark: All tests successful. No failed tests found.

Files Patch % Lines
codecov_auth/authentication/repo_auth.py 96.42% 1 Missing :warning:
upload/views/commits.py 83.33% 1 Missing :warning:

:loudspeaker: Thoughts on this report? Let us know!