github-starred/firejail

Fork 0

mirror of https://github.com/netblue30/firejail.git synced 2026-05-15 22:01:33 -06:00

[GH-ISSUE #4298] [meta] Avoid merging PRs that break CI #2613

New issue

Closed

opened 2026-05-05 09:16:48 -06:00 by gitea-mirror · 4 comments

gitea-mirror commented

2026-05-05 09:16:48 -06:00

Owner

Originally created by @kmk3 on GitHub (May 23, 2021).
Original GitHub issue: https://github.com/netblue30/firejail/issues/4298

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)

Originally created by @kmk3 on GitHub (May 23, 2021). Original GitHub issue: https://github.com/netblue30/firejail/issues/4298 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)

gitea-mirror closed this issue

2026-05-05 09:16:49 -06:00

gitea-mirror commented

2026-05-05 09:16:51 -06:00

Author

Owner

@kmk3 commented on GitHub (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] https://github.com/netblue30/firejail/pull/4229

@kmk3 commented on GitHub (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] https://github.com/netblue30/firejail/pull/4229

gitea-mirror commented

2026-05-05 09:16:52 -06:00

Author

Owner

@reinerh commented on GitHub (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.

@reinerh commented on GitHub (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.

gitea-mirror commented

2026-05-05 09:16:53 -06:00

Author

Owner

@kmk3 commented on GitHub (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 commented on GitHub (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.

gitea-mirror commented

2026-05-05 09:16:53 -06:00

Author

Owner

@kmk3 commented on GitHub (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.

@kmk3 commented on GitHub (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.

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: github-starred/firejail#2613

No description provided.

Rows
Columns