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

don't error on missing results*.json #398

Merged

Conversation

@IanButterworth IanButterworth added the automerge Kodiak will auto-merge this PR once all CI is green. (This label was previously called "merge me".) label Oct 24, 2024
@IanButterworth
Copy link
Member Author

Can we make Test not Required?

Generally I think this repo might just better suit requiring 1 other approval, rather than required runs, given that the julia repo could be broken. (I'm expecting the windows run to timeout again, in 3h, which is tedious).

Or can someone admin merge.

@IanButterworth
Copy link
Member Author

@DilumAluthge perhaps?

@DilumAluthge DilumAluthge merged commit 3224390 into JuliaCI:main Oct 24, 2024
4 of 6 checks passed
@DilumAluthge
Copy link
Member

I've admin merged.

@DilumAluthge
Copy link
Member

I'm reluctant to relax the required status checks here, just because there have been cases where a PR to this repo looks good, and it is merged with incomplete CI results, but then breaks JuliaLang/julia. In this case, JuliaLang/julia was good, and then the bad PR merged in this repo breaks JuliaLang/julia.

But then as @IanButterworth points out, the counter-case is when JuliaLang/julia is already broken, and thus a fast merge is needed here to fix JuliaLang/julia.

@DilumAluthge
Copy link
Member

The problem with removing Test from the list of required status checks is that it will have no impact unless the summary status (buildkite/julia-buildkite) is also removed from the list of required status checks.

And I'm not thrilled about the idea of removing buildkite/julia-buildkite from the list of required status checks. We had a very interesting case a while back where all Buildkite statuses were green on a PR except for the buildkite/julia-buildkite status. We couldn't figure out why, and we were all tempted to just merge the PR, because we assumed the buildkite/julia-buildkite status was being flaky or something. Eventually I managed to diagnose it - there was an actual bug in the PR (that would have broken CI on JuliaLang/julia), and the only way we detected it was by having buildkite/julia-buildkite required.

It's definitely a tough balance: on one hand, I want to avoid introducing new breakage to JuliaLang/julia, but on the other we need to be able to quickly fix existing breakage on JuliaLang/julia.

Perhaps we can keep the required status checks as-is, and maybe expand the pool of people that can admin-merge when there is a time-sensitive PR (like this one) that needs to be merged with incomplete CI checks?

@IanButterworth IanButterworth deleted the ib/dont_error_missing_results branch October 24, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Kodiak will auto-merge this PR once all CI is green. (This label was previously called "merge me".)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants