Count me as -1 on this.

Strict rules like these tend to get in the way of getting work done when you need it.

I totally agree that we need safeguards against pushing stuff that breaks CI, but we seem to have plenty already: we use code review thoroughly, and that triggers CI; and if one ever pushes something that breaks CI to master, they get a notification that that happened, so that one can (if everything else fail, immediately git revert). IMHO that's enough. YMMV.

I totally agree that we need safeguards against pushing stuff that breaks CI, but we seem to have plenty already: we use code review thoroughly, and that triggers CI;

yes.

And systematically, as a reviewer, prior to accepting a diff or not, i feel obliged to take a look at whether the build is red relevantly or not (because it sometimes is not, like your morning diffs ;)

I'm getting tired of the pattern "please try and unbreak the ci if relevant" and "checking the ci's state".
If i accept immediately in the current state, people can push and break the ci in master... (which i don't want them to do, i'd like the ci green when possible prior to push).
If i don't accept immediately, i must remember to get back to it later (possibly someone else does the review as well, which might be good also, i'm not sure, we kinda duplicate the effort then).

In both cases, i have more load on my mind than i wish for...
On the other hand, if it's automated, i'm relieved of that burden (plus it's neutral).

For my part, it should be up to the author to ensure the ci is green (they can do so as they wish, including asking for help to others...).
And then, the review becomes simpler (which is should or else no one does it and diff tend to stay open forever) [1]

Granted, i could also stop being like me and just cut to the chase and "na, ci red, go fix it" but that's pretty much blunt and senseless [2].
I don't like that, i'm not a bot.
Let the ci do it, that's neutral thus fair to everyone.

Again, when i'm reviewing, I'd like to focus on the review (understanding the need for the code, its implementation, share and discuss insight, ...).
Not spend my time investigate and untangle whether the ci failed because the author somehow missed something (which is cool) or whether we actually had prior issues (which is less cool)... [1]

and if one ever pushes something that breaks CI to master, they get a notification that that happened,

No, once pushed, it does not.
It happens when opening/updating a diff, not after that.

We have no notifications (yet?) [3]
If we want to see the state after pushing, we either connect to the forge and check the repository's page or directly the ci (which i do and fix regularly [4], this is not enough though).

so that one can (if everything else fail, immediately git revert).

which we don't do...
Instead we usually fix the ci correctly... (which means delay on current tasks)

we seem to have plenty already

It's really not that much so far, the ci and the people willing to fix (when aware and if time allows it).

[1] By the way, having a way to determine whether the coverage moves in the right direction would be also great (to ease decision but that's another matter here).

[2] That's how i impose this on myself, when i open a diff, i make sure the ci is green (if not, i mark the diff as planning changes up until resolution if possible).
If it cannot be fixed, i explain the reasons (could be a crossed-repository dependency issue that phabricator does not know how to deal with, that happens).

[3] I don't know what quantity of work we need to actually have it.

[4] not this week though. I won't get side-tracked (too much) this week. I want to focus and reach the weekly goals set.

Cheers,

From what you wrote, what I think will actually fix your problem is seeing the OK/KO CI marker in the list of pending review requests, so that you can just skip the ones with failing build without having to click them. Would that be enough?

Then it will be the responsibility of who pushed the diff to fix the CI before asking others to review the code (as it should be, and as it is on github, gitlab, etc.).

About this:

and if one ever pushes something that breaks CI to master, they get a notification that that happened,

No, once pushed, it does not.

I've definitely got a "build failed" automatic notification after a push I did a while ago. I think it was on swh-environment, before we (recently) disabled the CI on it. If this notification behavior is not uniform across repos, we should make it so.

In T2012#37261, @zack wrote:

Count me as -1 on this.

Strict rules like these tend to get in the way of getting work done when you need it.

Oh, for completeness: what I'd like to avoid here is that the day you have to quickly land a commit in master due to $emergency, you *also* have to fight the CI that might be fubar at the same time due to Murphy law.

But if there is a way to, say, make git push fail if the CI is red but at the same time git push --force work (ignoring CI status), then this is fine by me and you can move me to +1.

I've definitely got a "build failed" automatic notification after a push I did a while ago. I think it was on swh-environment, before we (recently) disabled the CI on it. If this notification behavior is not uniform across repos, we should make it so.

Is that an email? (-> i opt-ed out from forge's email because too many).

Is that a forge notification, os notification? (-> i don't have notifications, that side-tracks me)

From what you wrote, what I think will actually fix your problem is seeing the OK/KO CI marker in the list of pending review requests, so that you can just skip the ones with failing build without having to click them. Would that be enough?

Well, it'd be a way forward yes.

Note that OK/KO won't be enough. We unfortunately need a 3rd state.
Technically some ci build won't work (at the moment, cross-dependency repository comes to mind).

Then it will be the responsibility of who pushed the diff to fix the CI before asking others to review the code (as it should be, and as it is on github, gitlab, etc.).

Totally aligned with that yes (up to the technical limitation i just mentioned)

Oh, for completeness: what I'd like to avoid here is that the day you have to quickly land a commit in master due to $emergency, you *also* have to fight the CI that might be fubar at the same time due to Murphy law.

heh, right [1]

[1] we could bypass the review process in that case though...(i don't like that either ;)

[2] Last time i got that emergency (loader-git foobar last friday).
In the end, the ci did not block me, the reviewers did... because the code could be better... (I added test which should have been there in the first place, but forgot we could use a better pytest fixture...)
It did block too long for a production issue emergency (+ deploy build time which is not that fast)

I finally landed stuff but it took way more time than needed...

Anyway, thanks a bunch for your input ;)