Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

[meta] Avoid merging PRs that break CI #4298

Closed
kmk3 opened this issue May 23, 2021 · 4 comments
Closed

[meta] Avoid merging PRs that break CI #4298

kmk3 opened this issue May 23, 2021 · 4 comments

Comments

@kmk3
Copy link
Collaborator

kmk3 commented May 23, 2021

As mentioned on #4297, CI is broken on master again, likely due to the merge
of #4229, which was already failing a check before being merged.

When a PR that breaks CI is merged, every subsequent commit appears to do so as
well, which makes good commits/PRs look like they break something and may let
broken commits get merged without being noticed (which defeats the purpose of
having a CI IMO).

Personally, both issues make me want to avoid opening PRs until that is fixed.

With that said, can we agree on not merging PRs that break CI, especially in
the case of basic checks like build_and_test?

The alternatives being to either fix the PR before merging or to change the CI
checks before merging if they're too onerous and/or pedantic. Note that in the
latter's case, ideally the PR branch would be rebased to master before merging,
so that the workflow runs again and checks that nothing else is broken.

Cc: @glitsj16 @netblue30 @reinerh @rusty-snake @smitsohu (as participants
of #4229 and #4256)

@kmk3
Copy link
Collaborator Author

kmk3 commented May 23, 2021

which makes good commits/PRs look like they break something

And may also break bisecting if that requires the usage of the same (or
similar) command to the one that breaks the check.

Notes on verification: If the checks do not appear on the main PR page (e.g.:
[1]) for some reason (e.g.: because it was already merged or if js is
disabled), they can be reached by clicking the "Checks" tab, or by just
appending "/checks" to the URL. If there's any red on the "donut chart" to the
left, it's broken.

[1] #4229

@reinerh
Copy link
Collaborator

reinerh commented May 23, 2021

I completely agree. MRs should only be merged when CI passes.

Should we maybe move this thread into a discussion? I'm not sure how this issue is actionable, or what the conditions are for closing it.

@kmk3
Copy link
Collaborator Author

kmk3 commented May 23, 2021

Should we maybe move this thread into a discussion? I'm not sure how this
issue is actionable, or what the conditions are for closing it.

That makes sense; I'll move it.

I'm not very familiar with the discussions feature, so I didn't think to open
it there.

@kmk3
Copy link
Collaborator Author

kmk3 commented May 23, 2021

Hmm I thought that "Convert to discussion" was a direct action, but it shows a
screen to pick the category. Not sure which one is appropriate (or if a new
one is needed); I'd guess "General" by the existing discussions.

Which one would you suggest? Feel free to just move it directly if you want.

@reinerh reinerh closed this as completed May 23, 2021
Repository owner locked and limited conversation to collaborators May 23, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants