1642939 - Add an option to land with DONTBUILD in the commit message

Status: RESOLVED FIXED

(Conduit :: Lando, enhancement, P3)

Description

[Marco Castelluccio [:marco]]

This way, instead of having to rename the commits after review, the developer can just land and Lando will take care of adding DONTBUILD.

Comment 1

[Zeid [:zeid]]

Attachment: GitHub Pull Request

Comment 2

[Andrew Halberstadt [:ahal]]

I have some reservations about this change, and I'm not sure we should be encouraging people to use DONTBUILD (I was actually thinking we should deprecate it entirely from the vcs backend).

There are two problems with DONTBUILD:

1. Pushes that use it can't have jobs backfilled or new jobs added. This makes it difficult for sheriffs.
2. They can cause us to skip backstops (if a DONTBUILD push's pushid happens to be a multiple of 10). This can limit the number of merge candidates sheriffs have to central as well as increase regression ranges.

Seeing that we are getting smarter and smarter about what gets scheduled for any given push, I think we should discourage its use and instead allow our algorithms to handle things (if a trivial patch ends up scheduling lots of stuff, then that is a bug we should fix).

My worry is that the new lando UI will encourage developers to use it, when instead we should be discouraging it.

Sorry for posting all this after the fact, I didn't know this was in the works :(. The change is well intentioned and if not for the problems mentioned above, would be a great idea!

Comment 3

[Andrew Halberstadt [:ahal]]

(In reply to Andrew Halberstadt [:ahal] from comment #2)

1. Pushes that use it can't have jobs backfilled or new jobs added. This makes it difficult for sheriffs.

Actually, looks like this isn't true.. so not as big a deal as I thought (but it'll still make backfilling slower for sheriffs due to no builds being present). My opinion is still that we should deprecate DONTBUILD.

Comment 4

[Simon Giesecke [:sg] [he/him]]

I think that the current UI for this is quite unfortunate: I almost accidentally enabled DONTBUILD when I indeed wanted to acknowledge a landing warning, which was almost immediately below that. Maybe it's possible to move that to a more remote position?

Comment 5

[Zeid [:zeid]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #4)

I think that the current UI for this is quite unfortunate: I almost accidentally enabled DONTBUILD when I indeed wanted to acknowledge a landing warning, which was almost immediately below that. Maybe it's possible to move that to a more remote position?

Fair enough, let me look into alternative UI options -- perhaps the section can be collapsed by default, placed elsewhere like you suggested, or changed into a button.

(In reply to Andrew Halberstadt [:ahal] from comment #2)

My worry is that the new lando UI will encourage developers to use it, when instead we should be discouraging it.

Perhaps :marco can chime in a little more here, but as I understand it this is already being done manually when needed, and we want to just reduce the friction.

Comment 6

[Andrew Halberstadt [:ahal]]

Yeah, it is done manually.. I guess my point was that I'd like people to stop doing it manually and so we should add more friction not less :).

Though if we can fix the issue where DONTBUILD causes us to skip backstop pushes, then I don't really care that much anymore. So maybe that's the best approach for us to take.

Comment 7

[Marco Castelluccio [:marco]]

(In reply to Andrew Halberstadt [:ahal] from comment #2)

I have some reservations about this change, and I'm not sure we should be encouraging people to use DONTBUILD (I was actually thinking we should deprecate it entirely from the vcs backend).

Seeing that we are getting smarter and smarter about what gets scheduled for any given push, I think we should discourage its use and instead allow our algorithms to handle things (if a trivial patch ends up scheduling lots of stuff, then that is a bug we should fix).

I understand your point, there is always a friction between "should we trust developers as they know more than our smart scheduler?" or "should we not trust developers as they can't have as complete a picture as a machine?". When I filed this, I was leaning towards the first. In cases like trivial changes to docs, we might not want to take the chance to let the machine choose what to run: we know that there are always some false positives, so we will run something needlessly. In the end though they will be very few, so it might not really be a large win.
So, I'm not really sure one way or the other.

I'd say if we intend to deprecate it, we can revert this change and we can WONTFIX bug 1657920.
If we don't intend to deprecate it, then let's fix bug 1657920 and let's keep this in to make the feature easier to use and more discoverable.

(In reply to Andrew Halberstadt [:ahal] from comment #3)

(In reply to Andrew Halberstadt [:ahal] from comment #2)

1. Pushes that use it can't have jobs backfilled or new jobs added. This makes it difficult for sheriffs.

Actually, looks like this isn't true.. so not as big a deal as I thought (but it'll still make backfilling slower for sheriffs due to no builds being present).

With bug 1657097, not all builds are present on all autoland pushes anyway. So this is not a big concern.

Comment 8

[Zeid [:zeid]]

Attachment: GitHub Pull Request

Comment 9

[Andrew Halberstadt [:ahal]]

(In reply to Marco Castelluccio [:marco] from comment #7)

(In reply to Andrew Halberstadt [:ahal] from comment #2)

I have some reservations about this change, and I'm not sure we should be encouraging people to use DONTBUILD (I was actually thinking we should deprecate it entirely from the vcs backend).

Seeing that we are getting smarter and smarter about what gets scheduled for any given push, I think we should discourage its use and instead allow our algorithms to handle things (if a trivial patch ends up scheduling lots of stuff, then that is a bug we should fix).

I understand your point, there is always a friction between "should we trust developers as they know more than our smart scheduler?" or "should we not trust developers as they can't have as complete a picture as a machine?". When I filed this, I was leaning towards the first. In cases like trivial changes to docs, we might not want to take the chance to let the machine choose what to run: we know that there are always some false positives, so we will run something needlessly. In the end though they will be very few, so it might not really be a large win.
So, I'm not really sure one way or the other.

For cases like docs we don't have to trust a machine because we can add hardcoded SCHEDULES rules. Also, if developers are always using DONTBUILD, it means we won't know which cases we have missed and need to fix :p (though in practice we haven't exactly been aggressively hunting these down either).

I'd say if we intend to deprecate it, we can revert this change and we can WONTFIX bug 1657920.
If we don't intend to deprecate it, then let's fix bug 1657920 and let's keep this in to make the feature easier to use and more discoverable.

I'm in camp "let's get rid of it". However fixing bug 1657920 might actually be a lot easier than trying to get rid of it (there are stakeholders and use cases we need to consider for the latter). So it's likely going to be worth fixing bug 1657920 in the short term and then having a proper conversation about deprecating it on our own time.

Comment 10

[Andrew Halberstadt [:ahal]]

(In case it wasn't clear, I think it's fine if we leave this bug landed for now)

Comment 11

[Julian Descottes [:jdescottes]]

It could be nice to add some documentation on what DONTBUILD does, when it should be used (maybe in https://searchfox.org/mozilla-central/source/devtools/docs/contributing/landing-code.md ?) and link to it from Lando?

Comment 12

[:glob ✱]

(In reply to Julian Descottes [:jdescottes] from comment #11)

It could be nice to add some documentation on what DONTBUILD does, when it should be used (maybe in https://searchfox.org/mozilla-central/source/devtools/docs/contributing/landing-code.md ?) and link to it from Lando?

That's bug 1658019