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.

Sign up for GitHub

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

Jump to bottom

ci: fix codecov upload issue #3421

Merged
merged 4 commits into from
May 20, 2024
Merged

ci: fix codecov upload issue #3421

merged 4 commits into from
May 20, 2024

Conversation

shahar-h
Copy link
Contributor

What this PR does / why we need it:
Fixed codecov upload issue by configuring authentication with oidc.
In case PR is from fork OIDC will not work since PRs from forks have read permission on id-token.
Anyway Codecov supports Token-less Uploads for PRs from forks.
Also set fail_ci_if_error back to true.

['error'] There was an error running the uploader: Error uploading to [https://codecov.io:](https://codecov.io/) Error: There was an error fetching the storage URL during POST: 429 - {'detail': ErrorDetail(string='Rate limit reached. Please upload with the Codecov repository upload token to resolve issue. Expected time to availability: 2032s.', code='throttled')}

Which issue(s) this PR fixes:

Fixes #

@shahar-h
ci: fix codecov upload issue
859fb39 
Signed-off-by: Shahar Harari <shahar.harari@sap.com>
@shahar-h shahar-h requested a review from a team as a code owner May 17, 2024 15:07
@codecov Codecov
Copy link

codecov bot commented May 17, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.41%. Comparing base (c4b0216) to head (f8befcd).
Report is 12 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3421      +/-   ##
==========================================
+ Coverage   66.97%   67.41%   +0.43%     
==========================================
  Files         164      166       +2     
  Lines       23882    19184    -4698     
==========================================
- Hits        15994    12932    -3062     
+ Misses       6964     5322    -1642     
- Partials      924      930       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shahar-h
remove trailing space
18e9da7 
Signed-off-by: Shahar Harari <shahar.harari@sap.com>
@shahar-h
Copy link
Contributor Author

Works as expected:

info - 2024-05-17 15:18:33,306 -- The PR is happening in a forked repo. Using tokenless upload.

@shahar-h shahar-h mentioned this pull request May 17, 2024
Closed
@shahar-h
Copy link
Contributor Author

/retest

arkodg
arkodg reviewed May 17, 2024
View reviewed changes
.github/workflows/build_and_test.yaml Outdated
# only use oidc for push events or PRs from the same repo, since PRs from forks cannot fetch an OIDC token
# Codecov supports Token-less Uploads for PRs from forks:
# https://docs.codecov.com/docs/codecov-uploader#supporting-token-less-uploads-for-forks-of-open-source-repos-using-codecov
use_oidc: ${{ github.event_name == 'push' || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

should we keep this simple - event is push ? so we encourage PRs via forks only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@shahar-h
simplify condition
9618693 
Signed-off-by: Shahar Harari <shahar.harari@sap.com>
@shahar-h
Merge branch 'main' into fix-codecov-upload
f8befcd
arkodg
arkodg approved these changes May 17, 2024
View reviewed changes
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

@arkodg arkodg requested review from a team May 17, 2024 20:12
@shahar-h
Copy link
Contributor Author

According to codecov/codecov-action#1404 use_oidc should be ignored on pull request from forks, and instead tokenless request should be sent. That's what I did in my previous PR and it failed: https://github.com/envoyproxy/gateway/actions/runs/9129469887/job/25104103828?pr=3419.
Opened an issue.

@shahar-h
Copy link
Contributor Author

/retest

@shahar-h
Copy link
Contributor Author

@arkodg we can switch to use_oidc: true once codecov/codecov-action#1437 is merged and a new codecov version is released.

@shahar-h
Copy link
Contributor Author

shahar-h commented May 19, 2024 •
edited
Loading

@arkodg this won't work for dependabot PRs since they are not from fork, so I suggest to go back to the previous condition:

use_oidc: ${{ github.event_name == 'push' || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository) }}

Once codecov-action fix will be merged we can change that to true.

shawnh2
shawnh2 approved these changes May 20, 2024
View reviewed changes
Copy link
Contributor

@shawnh2 shawnh2 left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for the fix

@zirain zirain merged commit e847d68 into envoyproxy:main May 20, 2024
23 checks passed
@shahar-h
Copy link
Contributor Author

@zirain @shawnh2 this won't work for dependabot PRs as I mentioned here. I'll open a new PR to fix that.

@shahar-h shahar-h mentioned this pull request May 20, 2024
Merged
@shahar-h
Copy link
Contributor Author

See #3439

@shahar-h shahar-h deleted the fix-codecov-upload branch May 20, 2024 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shawnh2 shawnh2 shawnh2 approved these changes

@arkodg arkodg arkodg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shahar-h @arkodg @shawnh2 @zirain