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

Stop using the deceiving continue-on-error #2547

Closed
wants to merge 1 commit into from

Conversation

pirj
Copy link
Member

@pirj pirj commented Dec 30, 2021

Build jobs appear as green even though they actually fail, e.g. #2537 (comment)

A better approach is to mark jobs as "required" in branch protection.
image

This way, failed jobs are red, but this doesn't prevent the PR from being merged upon at maintainers' discretion.
image

@JonRowe Can you please set up branch protection so it has 6.1 jobs and below as "Required"?

@pirj pirj self-assigned this Dec 30, 2021
@pirj pirj requested a review from JonRowe December 30, 2021 12:58
@pirj pirj marked this pull request as draft December 30, 2021 13:25
@pirj pirj force-pushed the replace-continue-on-error-with-branch-protection branch from 57187e9 to 2149fcc Compare December 30, 2021 16:32
Build jobs appear as green even though they actually fail.
A better approach is to mark jobs as "required" in branch protection.
This way failed jobs are red, but this doesn't prevent the PR from being
merged upon at maintainers' discretion.
@pirj pirj force-pushed the replace-continue-on-error-with-branch-protection branch from 2149fcc to bce49d8 Compare December 30, 2021 17:10
@pirj pirj marked this pull request as ready for review December 30, 2021 17:10
@pirj
Copy link
Member Author

pirj commented Dec 30, 2021

Green for Rails 6.1 and below, red for Rails 7.0 and edge, as expected.

@JonRowe JonRowe closed this Dec 31, 2021
@JonRowe
Copy link
Member

JonRowe commented Dec 31, 2021

The problem with this, and with making status checks required, is it prevents us from having an "allow failures" mode, which as you can see by the red builds, we need for edge rails and currently unsupported rails versions.

My understanding is setting the builds to required, would actually prevent this PR from being merged for example.

@pirj pirj deleted the replace-continue-on-error-with-branch-protection branch December 31, 2021 11:03
@pirj
Copy link
Member Author

pirj commented Dec 31, 2021

setting the builds to required, would actually prevent this PR from being merged for example.

Yes. I suggest marking those jobs that we know should pass, e.g. currently Rails 6.1 on Ruby 2.7.
Others, like Rails 7 on Ruby 3.1 should not be marked as such until we make sure it's stable.

In any case, you can still merge a PR with a red "required" build job, but GH will ask for an additional confirmation:
image

A benefit, as I see it, is that non-required builds are shown as red.
This may save us some trouble with regressions in Rails 7 support when we get to it. With the current approach, the build will appear as "all green", even if the PR broke Rails 7 support that was previously green.
An easy way to check this it to unfold jobs with "Show all checks", and check build times for suspiciously low "Successful in 2m" where the baseline is ~15m.
image

@JonRowe
Copy link
Member

JonRowe commented Jan 6, 2022

A benefit, as I see it, is that non-required builds are shown as red.

Thats not a benefit to me, we want builds to be green if required builds pass, github doesn't give us the option of having optional failures without this, those interested in non-required builds (mostly us as we develop compatibility for things) can investigate individual builds when we care, it would be best if @github allowed us to mark non required builds but thats not a feature they've built yet.

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.

None yet

2 participants