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

[bug, CI] fix issues with forks, force green, and concurrency in codecov report uploading #2225

Merged
merged 5 commits into from
Dec 13, 2024

Conversation

icfaust
Copy link
Contributor

@icfaust icfaust commented Dec 13, 2024

Description

This PR fixes 4 issues with the recent codecov roll-out, where fork testing did not cover all scenarios occurring in the main repo:

  1. Most importantly, it fixes issues with pull requests, where PRs from forks are not being properly uploaded to codecov (and defaulting to main). Unfortunately default github webhooks for workflow_run are not complete enough to get the requisite pull request information unless the PR comes from the repo itself. A workaround uses the github cli along with other more reliable aspects of the webhook (branch name, name and full_name) to construct the necessary information to get the PR number.

  2. Default settings of codecov can lead to failures in CI. As discussed internally, we would like this not to be enforced to start. A configuration yaml file is added to .github with very conservative settings. In the case that enforcement will be added, all the necessary information is already included.

  3. Concurrency protection is not necessary for this github action: it is a service to PRs and pushes by accomplishing higher-security tasks (i.e. with secrets) on their behalf without using the PR/push code (see ultralytics hack from this week as to why pull_request_target must be avoided). Multiple PRs may run the same action at the same time, and so concurrency is a feature.

  4. If multiple pushes to main occur at the same time, it was using only the latest SHA which comes from the repo, which will not properly map into codecov, instead use the head_ref SHA so that the older pushes maintains their SHAs.

See:
icfaust#8
icfaust#7


PR should start as a draft, then move to ready for review state after CI is passed and all applicable checkboxes are closed.
This approach ensures that reviewers don't spend extra time asking for regular requirements.

You can remove a checkbox as not applicable only if it doesn't relate to this PR in any way.
For example, PR with docs update doesn't require checkboxes for performance while PR with any change in actual code should have checkboxes and justify how this code change is expected to affect performance (or justification should be self-evident).

Checklist to comply with before moving PR from draft:

PR completeness and readability

  • I have reviewed my changes thoroughly before submitting this pull request.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes or created a separate PR with update and provided its number in the description, if necessary.
  • Git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details).
  • I have added a respective label(s) to PR if I have a permission for that.
  • I have resolved any merge conflicts that might occur with the base branch.

Testing

  • I have run it locally and tested the changes extensively.
  • All CI jobs are green or I have provided justification why they aren't.
  • I have extended testing suite if new functionality was introduced in this PR.

@icfaust icfaust changed the title [bug, CI] fix issues with forks, concurrency in codecov report uploading [bug, CI] fix issues with forks, force green, and concurrency in codecov report uploading Dec 13, 2024
@icfaust icfaust merged commit ded94bf into uxlfoundation:main Dec 13, 2024
27 checks passed
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.

2 participants