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

Set fail_ci_if_error flag to true #6372

Merged
merged 2 commits into from
May 2, 2024

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented May 1, 2024

In a recent PR, we got reports from CodeCov about several lines not being covered. After further inspection, I found an error message in the coverage file upload saying we hit a rate limit.
This will make it more likely we spot such issues in the future.

@greg0ire greg0ire added the CI label May 1, 2024
@greg0ire
Copy link
Member Author

greg0ire commented May 1, 2024

I expect the first build here to be red, and after that I will follow the instructions at https://docs.codecov.com/docs/adding-the-codecov-token and then it should be green 🤞

@greg0ire

This comment was marked as resolved.

@greg0ire

This comment was marked as resolved.

@greg0ire
Copy link
Member Author

greg0ire commented May 1, 2024

Finally! A build that fails because of Codecov

@greg0ire
Copy link
Member Author

greg0ire commented May 1, 2024

Ah but secrets are just not passed to workflows triggered from a fork, so there is no way this is going to work, is there? 😞

@greg0ire greg0ire marked this pull request as draft May 1, 2024 16:02
@greg0ire
Copy link
Member Author

greg0ire commented May 1, 2024

Looks like v4 addresses this ?

@greg0ire greg0ire force-pushed the sensible-status-code-codecov branch from eb26cac to 02f0276 Compare May 1, 2024 16:06
@greg0ire greg0ire marked this pull request as ready for review May 1, 2024 16:24
@greg0ire
Copy link
Member Author

greg0ire commented May 1, 2024

86.54% (+19.54%) compared to b05e48a

Wonderful increase 😁

Looking at the CodeCov UI, it says there is only 1 upload, but when I download it, I can see it contains several reports.

Note that we might have to specify CODECOV_TOKEN for builds not emanating from a fork. Let's see what happens after merging this? If there is an issue, I will make another PR, this time from a branch I would push directly to this repository.

greg0ire added 2 commits May 1, 2024 23:21
In a recent PR, we got reports from CodeCov about several lines not
being covered. After further inspection, I found an error message in the
coverage file upload saying we hit a rate limit.
This will make it more likely we spot such issues in the future.

The PR: doctrine#6370
@greg0ire greg0ire force-pushed the sensible-status-code-codecov branch from 02f0276 to 27eb261 Compare May 1, 2024 21:22
Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

I'd expect that error to appear again. Should codecov upload on a PR? What about having just a coverage % output on a PR and the upload on merges to the branches? Food for thoughts if another PR is needed.

@greg0ire
Copy link
Member Author

greg0ire commented May 2, 2024

Should codecov upload on a PR?

IMO it can be useful for us to see what part of a patch is uncovered before merging, so yes. If the error appears again, that won't prevent a merge, and it's IMO better for us to be aware of the issue rather than getting lots of comments about lines being uncovered.

@greg0ire greg0ire merged commit ba7dbe1 into doctrine:3.8.x May 2, 2024
88 checks passed
@greg0ire greg0ire deleted the sensible-status-code-codecov branch May 2, 2024 05:57
@greg0ire greg0ire added this to the 3.8.5 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants